-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[Java] Add CharacterLiteral to CompileTimeConstantExpr.getStringValue #8325
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
[Java] Add CharacterLiteral to CompileTimeConstantExpr.getStringValue #8325
Conversation
int paren = (12); | ||
String string = "a string"; | ||
int ternary = (3 < 5) ? 1 : 2; | ||
char charLiteral = 'a'; | ||
|
||
return; | ||
} |
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.
Why is this file tab delimited when the rest are space delimited? 🤦
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 don't want to fix this as a part of this PR 😆 someone might want to make a sweep though, just my 10c
18b8a91
to
4f23a93
Compare
4f23a93
to
04cd0db
Compare
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.
Hey @JLLeitschuh, thanks for your contribution. This mostly LGTM, I added a couple of inline comments.
Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com>
This PR is done and can be merged whenever you're comfortable doing so. 😄 |
This change breaks string concatenation logic for It looks like the reason why this change to |
Hm, sure, the concat case clearly needs fixing. Concat of string + char is nice to have though for example. @JLLeitschuh how about amending to have a separate getStringifiedValue, and use that to handle concatenation in the particular case where the resulting expression is of type String? Thus
|
To me that looks acceptable as well (if the qldoc describes that the value is converted to string), because in Java for example |
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue Resolves: - github#8325 (comment) - github#8325 (comment)
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue Resolves: - github#8325 (comment) - github#8325 (comment)
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue Resolves: - github#8325 (comment) - github#8325 (comment)
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue Resolves: - github#8325 (comment) - github#8325 (comment)
Removes CharacterLiteral from CompileTimeConstantExpr.getStringValue Resolves: - github#8325 (comment) - github#8325 (comment)
Thanks for the feedback! Resolved here: #8360 |
Java: Revert #8325, Add CharacterLiteral to CompileTimeConstantExpr.getStringValue
Simplifies end-user logic when looking for charater literals.