Refactor simple onOverrideMayBeNullExpr handlers
#747
Conversation
execution of the touched handler methods is inconsequential when parameter exprMayBeNull is already true
| && exprSymbol instanceof Symbol.MethodSymbol) { | ||
| return exprMayBeNull || isReturnAnnotatedNullable((Symbol.MethodSymbol) exprSymbol); | ||
| if (exprMayBeNull) { | ||
| return true; |
There was a problem hiding this comment.
like a preamble: "this handler only supports false -> true transitions"
| && isReturnAnnotatedNullable((Symbol.MethodSymbol) exprSymbol)) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
this is structured like:
if reason1_why_expr_may_be_null:
return true
if reason2_why_expr_may_be_null:
return true
return false
when there is only a single reason, we could of course use the condition in a single return statement if preferred?
the old style is imo more confusing (an has redundant checks):
if check1:
return exprMayBeNull || check2
return exprMayBeNull
if any of the checks have important side-effects (that i am not seeing) we should add tests that expose those imo.
There was a problem hiding this comment.
agreed this is an improvement here
| if (exprMayBeNull) { | ||
| return true; | ||
| } | ||
| if (expr.getKind() == Tree.Kind.METHOD_INVOCATION | ||
| && exprSymbol instanceof Symbol.MethodSymbol | ||
| && optionalIsGetCall((Symbol.MethodSymbol) exprSymbol, state.getTypes())) { | ||
| return true; |
There was a problem hiding this comment.
in the old code, this line returning true (and thus the whole if condition) only matters, if we wouldnt eventually be returning true with return exprMayBeNull anyway.
this again is assuming that the checks have no side-effects (if they do tests around it are missing)
| && isReturnAnnotatedNullable((Symbol.MethodSymbol) exprSymbol)) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
agreed this is an improvement here
msridhar
changed the title
onOverrideMayBeNullExpr handlersonOverrideMayBeNullExpr handlers
This reverts commit 7714c45.
This reverts commit 7714c45.
This reverts commit 7714c45.
This reverts commit 7714c45.
This reverts commit 7714c45.
execution of the affected handlers is inconsequential when parameter exprMayBeNull is already true