Fix Proc argument conversions when passing to binding #78
Conversation
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. |
|
|
||
| # Calls the *method*, using the *proc_name* to call-through to Crystal. | ||
| def build(method : Parser::Method, receiver = "self") : Call | ||
| def build(method : Parser::Method, receiver = "self", do_block = false) : Call |
There was a problem hiding this comment.
It generates snippets like Proc(Void).new do ... end instead of Proc(Void).new { ... }. This is because the template will be used inside the BindgenHelper.wrap_proc(%) template, which now comes directly from builtin_types.yml, and is a full template string that has access to environment variables. Thus Util.template will interpret the Proc body as an environment variable, unless the do notation is used.
Other solutions are to disable ENV access from from_crystal and friends, or support escape sequences for braces in Util.template.
There was a problem hiding this comment.
Ah ok, could you put what you just said into the docs of the method? Escaping-Support would be great, but that's for another day.
|
|
||
| # The default template. Uses `Util.template` to substitute *code* into the | ||
| # given *pattern*. | ||
| class Full < Base |
There was a problem hiding this comment.
I think the simple/full split adds more complexity than it saves. Just collapse them into one.
| end | ||
|
|
||
| # Compound template which runs *code* through two child templaters. | ||
| class Seq < Base |
There was a problem hiding this comment.
- Rename it to
Sequence,Seqisn't too often typed for the long form to be annoying (Or the short form to be memorable). - Use an array for input(s) to allow for 1..n instead of exactly two. In
#templatedo some#reducemagic.
| puts "#{"<<< Failed spec command:".colorize.mode(:bold)} #{command}" | ||
|
|
||
| raise Spec::AssertionFailed.new("Test for #{name}.yml failed, see above", source_file, source_start) | ||
| # raise Spec::AssertionFailed.new("Test for #{name}.yml failed, see above", source_file, source_start) |
There was a problem hiding this comment.
This is a mistake, will be fixed.
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! |
Why?
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. |
|
Closing this one in favour of #82. |
2 participants
Fixes #62. Also confirmed that it works on qt5.cr, with the exception that I need to alias
Qt::CrystalStringtoQt::Binding::CrystalString(somewhere Bindgen thinks thatCrystalStringisn't a binding type because it's configured as a built-in type).The solution is unsatisfactory; it expands the result of
CallBuilder::CrystalFromCppinsideCrystal::Pass#to_binding. The better way would be moving that part into a separateCallBuilderthen 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 ownCall::Body, hoping that CrystalProcs could be done in a similar pattern.) Another way is to extend the templating mechanism so that subclasses ofCall::Resultcan specialize the template behaviour.By the way, similar issues will probably arise in #76 when methods start passing
Slices around, another generic type likeProc. Any refactoring finished here will surely benefit that PR as well.The
Callhierarchy has always looked confusing to me; although everyCallrefers to oneParser::Method, the code bodies range from method invocation to entire code fragments. It seems they would fit into an even larger AST type hierarchy.