add new function type syntax (second try, closes #4799) #6667
Conversation
| @@ -573,6 +573,26 @@ enum ComplexType { | |||
| } | |||
|
|
|||
| /** | |||
| Represents an argument of a function type (`ComplexType.TFunction`). | |||
| **/ | |||
| typedef FunctionTypeArg = { | |||
There was a problem hiding this comment.
If we add a new type anyway we should consider adding the name position.
There was a problem hiding this comment.
I wanted to ask about that, don't we want to have generic PlacedName instead of adding namePos fields?
There was a problem hiding this comment.
I'm not sure. I think a flat structure is easier to handle, less breaking and even more performant (no nested object allocation).
There was a problem hiding this comment.
Should I also add typePos then?
TBH I wouldn't do that just now because adding positions to random types sounds really inconsistent. I would rather wait for some kind of parse tree integration to support precise positions and leave AST alone.
There was a problem hiding this comment.
My concern is that this will require people to update their macro code, and then if/when we add more positions, they have to update it again.
Also, we definitely have to make sure we don't lose the position in an encode/decode roundtrip. Check the other occurrences of encode_placed_name, they all come with a name_pos encoding as well. It's just not exposed on the Haxe side yet.
There was a problem hiding this comment.
Alright, I added namePos (should be that instead of name_pos for Haxe, right?)
|
I'll just copy-paste the nice gif from #6645 |
nadako
force-pushed
the
new_function_type_syntax2
branch
from
87d49b6
to
7fadad4
Compare
std/haxe/macro/TypeTools.hx
Outdated
| @@ -128,6 +128,7 @@ class TypeTools { | |||
| [ for (a in args) | |||
| { | |||
| name: a.name, | |||
| namePos: {file: "", min: -1, max: -1}, // TODO, I guess? | |||
There was a problem hiding this comment.
@Simn should we also use placed_name for TFun then?
There was a problem hiding this comment.
Not sure... it's kind of expected to lose some information when going typed -> untyped.
There was a problem hiding this comment.
Maybe we should make namePos optional for now, because for old syntax it doesn't even make sense...
There was a problem hiding this comment.
But the entire type is new, so why does it matter?
There was a problem hiding this comment.
Because there will be a lot of null_pos positions encoded for the old syntax. Maybe an optional field or a Null<Position> would be better for performance.
There was a problem hiding this comment.
I doubt that would even be measurable... but if it makes you feel better, I don't mind.
|
We can have a single CTFunction in Haxe compiler but we need to accept both old TFunction and new TFunctionArgs (or another name) in macro code, so we don't have to do a lot of #if BTW, is there #if haxe4 ? would be simplier than #if (haxe_ver >= 4) |
|
Closing this in favor of #6645 again (see discussions there). |
This also implements HaxeFoundation/haxe-evolution#23, but as discussed in #6645, this changes representation of arguments in
CTFunction(andhaxe.macro.Expr.TFunction).