Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luabind policies opinions #20

Open
decimad opened this issue Sep 6, 2013 · 7 comments
Open

luabind policies opinions #20

decimad opened this issue Sep 6, 2013 · 7 comments
Labels

Comments

@decimad
Copy link

decimad commented Sep 6, 2013

This is a message I would have liked to post on the luabind message list, but it doesn't seem to be maintained anymore so I can't get moderator approval.

Hello,

I'm currently working on a fork of luabind that gets rid of all boost
dependencies and makes use of c++11 unconditionally, so there are no
header-variadic-simulations left, which (aside from the enormous
boost/mpl includes) basically brought my visual studio to crawl and made
intellisense in any code using luabind pretty much unusable.
To put this into a context: I want to be able to add stuff like custom
indexer operators etc. and found that I just could not understand
luabind. It's utterly complex and its internals are pretty much
undocumented. So I'm basically applying transforms to a beast that I
don't yet understand completely while trying to maintain existing test
compatibility.

Currently I'm reworking the policy system. I found that luabind uses
policies for argument conversion (obviously necessary) and "call-static"
policies. It puts them together in one big list and when specific
policies are necessary, it searches in the policy list for policies.
This causes that converter policies are specialized for the occurance
index in call signatures, slow searches everywhere etc. That's why my
current attempt is having a converter-policy list that matches the
signature type list and a "call static" policy list. So my questions to
the ones that grasp current luabind's policy system: Do you see an
obvious problem in the long run with this pattern? Has any converter
policy ever been dependant on the argument index it was specialized for?
I could not find any occurance of this dependency but maybe I'm breaking
everything here.

With Kind Regards
Michael

@Oberon00
Copy link
Owner

Oberon00 commented Sep 6, 2013

This is a message I would have liked to post on the luabind message list, but it doesn't seem to be maintained anymore so I can't get moderator approval.

Although it really doesn't seem maintained, I was able to post messages without moderator approval at luabind-user.

"call-static" policies

I'm not sure what you mean. Could you give an example?

Has any converter policy ever been dependant on the argument index it was specialized for?

I'm not aware of any. In my opinion you are right -- it would be better to introduce an additional class conversion_policy_index<Index, Policy> which maps an index to a policy and is otherwise empty to make the conversion policies independent from the indices they are applied to.

That's why my current attempt is having a converter-policy list that matches the signature type list

That sounds like a good design to me, even better than a conversion_policy_index.

@decimad
Copy link
Author

decimad commented Sep 6, 2013

Hi there!
Thank you for answering. With "call static" I mean there are policies that are basically not dependant on any argument or a type of the signature of a call. They are basically just compile time static functors that are injected before a call is done and right after. Those are the ones you can stick into a policy cons which don't have an apply template for type (and index) specialization of the policy. They're called by specially tailored templates which filter them out of the combined policy list in invoke_member/invoke_normal.

Another question popped up:
I see that the function creators (template struct function_registration) which are created for a function call actually store a reference to the policy list and I cannot understand just why. The policy_cons only ever stored typedefs to my understanding, so the policy cannot store any state. But to my understanding every named object must have a distinct address, isnt the storage of the policy wasting a DWORD per functor there if we're only ever interested in its type?

Unfortunately the policy system is pretty deeply integrated and so my version is currently a big mess. I had not anticipated how much time it would take me to refactor this and all the attention that is necessary.
But my current results are that the intellisense source code browse base went down from 139mb to 75mb and this includes all the tests on which I have not worked at all, so it's worth pursuing this.
Can I ask back for questions regarding the patches you are applying so I can port them to my version? I don't have a schedule for when I'll be done, but then I would like to just patch up that refacored code base parallel to yours here on github. I don't have the slightes idea how much you guys have spent with the code base, but I guess I will not understand the problems being fixed for a rather long time being...

With Kind Regards
Michael

@Oberon00
Copy link
Owner

Oberon00 commented Sep 7, 2013

I see that the function creators (template struct function_registration) which are created for a function call actually store a reference to the policy list and I cannot understand just why.

As you can see in luabind/make_function.hpp the actual value of the passed Policies argument is even ignored later on so it indeed makes no sense. But it is also not that serious since function_registration objects are not permanently stored anywhere, they usually just exist between the square brackets of luabind::module.

Can I ask back for questions regarding the patches you are applying so I can port them to my version?

Sure.

@decimad
Copy link
Author

decimad commented Sep 10, 2013

Okay,
Policy stuff replaced and I'm failing only two tests now...

  1. "object_identity" which I cannot find in your sources and I don't know what the lua code there is supposed to mean -.-
  2. "unsigned int" where I don't know how to recover the missing bit that lua already got rid of apparently?
    There are some minor and majors things that should be cleaned up, but this thing is usable as it is.

My problem now is, that I did all the stuff outside of source control, I don't know why I did that, but that's the situation. I'll have to find a solution for that. Any ideas? I have thought about checking your fork out and just copy the hell over, but the problem is that my source was rpavlik's fork and not even the most recent one... I'd like my version to be a fork of some way so it's related to the original project, but I don't know where to snap in. When I sorted that out, I'd go about applying the patches I find in your commit list somehow.

With Kind Regards,
Michael

@Oberon00
Copy link
Owner

My problem now is, that I did all the stuff outside of source control, I don't know why I did that, but that's the situation. I'll have to find a solution for that. Any ideas?

The easiest way would probably be to first fork and clone the repository you based your work on. Then use git reset --hard <commit-id-you-based-it-on> to restore the right version, then overwrite the files with your changed ones. You can then use the usual git tools to make commits as fine-grained as you want (all at once, per file, or even parts of files with git add -p/-e per commit).
From then on, I'm not quite sure how to best proceed to merge in more commits, but one way would be to simply use git pull to merge in all commits you discarded with git reset. Another way (or the next step) would be to add as remote what you want to merge and then do git fetch <remote-name> and git merge/(at the branch you want to merge it in). To push your work to github you will then have to use the--forceor-fargument forgit push`.

For the failing tests: They probably failed even before your changes. 1) is removed because I thought that it cannot possibly work (but in fact it should, although possibly the test requires some changes). 2) is fixed with 6f0651d.

@decimad
Copy link
Author

decimad commented Sep 11, 2013

Hi there, thank you so much!
I need to do other stuff and fix up some #pragma onces etc. but will try to get that going tonight. The situation is not as bad as I had anticipated, since I based my version on rpavlik's master from 31th of august, so its reasonably recent. However, I touched MANY files of the public and internal interface while replacing the policy_cons-system and replace most of the boost stuff with c++11's versions, so it's hard to create fine grained commits that wouldn't break the build. If I had done this now, I would have done all of this in three independent incremental steps, but this was my first time working on such a beast, I didn't know better :(
As a matter of fact, I stayed with converter_policy_injector< N, Type >-style until the invoke, as there are many paths leading there and I would have had to convert that to a complete list on every one of them. In the public interface specifying a complete list is unreasonable for user of course otoh. More on that in a list-form when I commit.
How to format the code is another thing... I recently saw, that clang has been touched up to run on windows and specifically there's also a VS-plugin that does the code reformat that Chandler demonstrated on Going Native 2013. Wouldn't that be nice to use for a automatic and consistent formatting?

@decimad
Copy link
Author

decimad commented Sep 18, 2013

Alright, I'm sorry for the rather long absence, I had two unenjoyable code sprints that held me back until today.
I've just uploaded a preliminary state of the fork (which is intended for VS RC 2013+). Many changes, still the policy system is awkward, put mildly. I think I will iterate over it when I get a better hang of it.
I'll gladly accept any suggestions! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants