-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Deprecate PrimitiveType.getADefaultValue()
#6796
base: main
Are you sure you want to change the base?
Java: Deprecate PrimitiveType.getADefaultValue()
#6796
Conversation
or | ||
// The default value for primitive types. | ||
init = t.(PrimitiveType).getADefaultValue() | ||
isDefaultValueLiteral(init, t) |
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 looks dodgy from a performance point-of-view. Not as much the code change, but rather, this already looked dodgy before the change. So since it probably worked then it's likely that this depends on decent magic, and the changed code will still depend on magic, but since this isn't a certainty, we should verify this before merging (or update the code to not be so full of cartesian products).
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'm ok with merging if we can verify that the optimiser pushes relevant magic into both predicates to eliminate the CPs.
@@ -27,13 +27,24 @@ predicate flowStep(Expr decl, Expr init) { | |||
decl.(CastExpr).getExpr() = init | |||
} | |||
|
|||
predicate isDefaultValueLiteral(Literal l, Type t) { |
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.
To resolve the cartesian product worry (all reftypes X all nulls), how about just omitting the Type column? This could be just:
Don't flag Expr e if e is (a) null, (b) zero, or (c) of integral type and negative one. Note this actually expands the query slightly to permit Integer i = 0;
, but considering we already permit Integer i = -1;
then we should probably accept that anyhow.
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.
Sorry for the latest response. Was this a suggestion for me or directed to aschackmull, or both of us?
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.
Both. @aschackmull do you think that's a sensible solution?
Resolves #6615
See #6615 (comment)