Papierkorb / bindgen

Binding and wrapper generator for C/C++ libraries
GNU General Public License v3.0
179 stars 18 forks source link

Fix Proc argument conversions when passing to binding #78

Closed HertzDevil closed 4 years ago

HertzDevil commented 4 years ago

Fixes #62. Also confirmed that it works on qt5.cr, with the exception that I need to alias Qt::CrystalString to Qt::Binding::CrystalString (somewhere Bindgen thinks that CrystalString isn't a binding type because it's configured as a built-in type).

The solution is unsatisfactory; it expands the result of CallBuilder::CrystalFromCpp inside Crystal::Pass#to_binding. The better way would be moving that part into a separate CallBuilder then adding the Crystal wrapper call inside the Qt processor, but then the conversions would only work within that processor. (This was the original plan, which explains why I tried to move the lambda expression stuff into its own Call::Body, hoping that Crystal Procs could be done in a similar pattern.) Another way is to extend the templating mechanism so that subclasses of Call::Result can specialize the template behaviour.

By the way, similar issues will probably arise in #76 when methods start passing Slices around, another generic type like Proc. Any refactoring finished here will surely benefit that PR as well.

The Call hierarchy has always looked confusing to me; although every Call refers to one Parser::Method, the code bodies range from method invocation to entire code fragments. It seems they would fit into an even larger AST type hierarchy.

Papierkorb commented 4 years ago

It seems they would fit into an even larger AST type hierarchy

That is correct. The reason is simply that I didn't want to model a full AST for C++ and Crystal each (even one would be crazy). So it's a compromise between having a partial one to model the overall structure but not individual expressions.

Papierkorb commented 4 years ago
  1. I think the PR is too large. I like the concept of the proper Template module, much better than calling Util.template everywhere. Could you split that off into its own PR? (The review comments are primarily on the templating code)
  2. As you have obviously put a lot of time into this, could you tell us your thoughts which got you to your decision(s)? I want to better understand your thought process on this.

I'm not saying (or thinking) that these changes are bad – Especially if they make it work, I mean, it works so it works right? I'm simply weary as to merge this PR without being completely in the loop. Thank You!

Papierkorb commented 4 years ago

and Slice will probably be another one

Why?

I may take a look at the possibility of a language-agnostic AST representation later. (The Templates model a linear chain of AST nodes, but only for Call::Result objects.)

Without clear evidence that this helps a ton in the general case I don't see the need much. For Bindgen, due to what it's doing, it's hard to strike a balance between under-engineering and over-engineering and keeping complexity in check while it all still being readable. If you intend to do something in that area, please open a conversation beforehand.

HertzDevil commented 4 years ago

Closing this one in favour of #82.