-
Notifications
You must be signed in to change notification settings - Fork 123
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
Fix synth literal function for enums #1113
Fix synth literal function for enums #1113
Conversation
clang-tidy review says "All clean, LGTM! 👍" |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1113 +/- ##
==========================================
+ Coverage 94.21% 94.24% +0.02%
==========================================
Files 48 48
Lines 8131 8164 +33
==========================================
+ Hits 7661 7694 +33
Misses 470 470
... and 4 files with indirect coverage changes
|
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.
Can we add a test for it?
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
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.
LGTM!
Result = clad::synthesizeLiteral( | ||
dyn_cast<EnumType>(QT)->getDecl()->getIntegerType(), C, APVal); | ||
SourceLocation noLoc; | ||
Expr* cast = CXXStaticCastExpr::Create( |
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.
Can we please use clang::Sema::BuildCXXNamedCast
here? BuildCXXNamedCast
will automatically determine the correct values for ExprValueKind
, CXXCastPath
, and FPOptionsOverride
.
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.
This regards the whole function and how it's implemented. For each case we create the literal manually, but we can alter that to include Sema as an arg instead of the AST context and use Sema's Build functions for all type cases. @vgvassilev what's your opinion on this?
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.
Yeah, that'd be annoying to update all call sites. Probably worth doing but maybe after the other PR is complete and merged?
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.
Sounds good
Tested in PR #1111