Skip to content

Conversation

@Marcono1234
Copy link
Contributor

Resolves #6615

See #6615 (comment)

or
// The default value for primitive types.
init = t.(PrimitiveType).getADefaultValue()
isDefaultValueLiteral(init, t)
Copy link
Contributor

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).

Copy link
Contributor

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.

decl.(CastExpr).getExpr() = init
}

predicate isDefaultValueLiteral(Literal l, Type t) {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Java: PrimitiveType.getADefaultValue() is misleading and might not work correctly

3 participants