-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add C-Style casts support #26
Add C-Style casts support #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. Thanks for moving the models to a separate file.
|
||
if (foundCast && part is MemberAccess memberAccess) | ||
{ | ||
array[i] = new ConditionalAccess(memberAccess); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to keep the foundCast
set to true
? Shouldn't we "reset" it after the first member access"? Also when there is a ConditionalAccess
, shouldn't that reset the flag too?
new ConditionalAccess(new MemberAccess("X")), | ||
new ConditionalAccess(new MemberAccess("Y")), | ||
new ConditionalAccess(new MemberAccess("Z")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct transformation would be turning just the first MemberAccess
into a conditional one. Or am I missing something?
return TransformExplicitCastsToAsCasts(source); | ||
} | ||
|
||
private static CodeWriterBinding TransformExplicitCastsToAsCasts(CodeWriterBinding source) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we want to move this code to a separate class. I see value in being able to test this separately, but it seems too complicated for this use case. If we were planning on adding more transforms, it might be great way to do this, but I don't think we'll add more. I would personally implement a similar loop in AppendHandlersArray
to correctly insert ?
and test this using integration tests. What do you think?
a573668
into
simonrozsival:binding-source-generator
No description provided.