Community
Participate
Working Groups
Fup of bug 250056. See bug 250056 comment 11.
Created attachment 160043 [details] proposed fix v0.5 + regression tests The approach is : if any variable is checked against null inside an assert expression, we encode this information in the array UnconditionalFlowInfo$nullStatusChangedInAssert by setting a bit corresponding to its position. This is done via UnconditionalFlowInfo#markedAsNullOrNonNullInAssertExpression(LocalVariableBinding). So everytime we encounter an if statement where there is a redundant or always false null check , we check if the variable had been marked as null or non null in an assert expression, and if it is, then we dont set the UNREACHABLE bit to optimize code gen. We do this checking via UnconditionalFlowInfo#isMarkedAsNullOrNonNullInAssertExpression(LocalVariableBinding). UnconditionalFlowInfo#combineNullStatusChangeInAssertInfo(UnconditionalFlowInfo otherInits) is a helper method which merges the array UnconditionalFlowInfo$nullStatusChangedInAssert in two separate flowInfos. Also note that the Dead Code warnings no longer appear inside if else blocks in which the condition contains a local variable which was compared with null in an earlier assert expression. This makes sense because just like we dont want to optimize code gen, we also dont want to report over-aggressive dead code warnings when asserts are disabled. All changes in NullReferenceTests pertain to this.
Patch looks good. I'll release it later today. I'll rewrite a bit the constants in FlowContext to look like: public static final int CAN_ONLY_NULL_NON_NULL = 0x0000; //check against null and non null, with definite values -- comparisons public static final int CAN_ONLY_NULL = 0x0001; //check against null, with definite values -- comparisons public static final int CAN_ONLY_NON_NULL = 0x0002; //check against non null, with definite values -- comparisons public static final int MAY_NULL = 0x0003; // check against null, with potential values -- NPE guard public static final int CHECK_MASK = 0x00FF; public static final int IN_COMPARISON_NULL = 0x0100; public static final int IN_COMPARISON_NON_NULL = 0x0200; // check happened in a comparison public static final int IN_ASSIGNMENT = 0x0300; // check happened in an assignment public static final int IN_INSTANCEOF = 0x0400; // check happened in an instanceof expression public static final int CONTEXT_MASK = ~CHECK_MASK; I never really liked multiple field declarations as in current codebase.
(In reply to comment #2) > I'll rewrite a bit the constants in FlowContext to look like: Ok. BTW, i forgot to ask one thing. In the 4 new tests introduced in ConformTest.java, the disassembled output contains a different number for diff compliance levels for the method foo, ie. basically the first line of the output - "Method descriptor #x" where x is different for compliance level 1.4, 1.5 and 1.6. I couldnt find an explanation for it, though I'm pretty sure its not a bug or anything. Do you have an idea why this happens?
(In reply to comment #3) > BTW, i forgot to ask one thing. In the 4 new tests introduced in > ConformTest.java, the disassembled output contains a different number for diff > compliance levels for the method foo, ie. basically the first line of the > output - "Method descriptor #x" where x is different for compliance level 1.4, > 1.5 and 1.6. I couldnt find an explanation for it, though I'm pretty sure its > not a bug or anything. Do you have an idea why this happens? In 1.4 the code generation for the assert statement ends up with different bytecodes. In 1.4, the synthetic bit is an attribute and in other versions it is a modifier. This has a side-effect on the reference into the constant pool (indexes becomes slightly different). For difference between 1.5 and 1.6, it comes from the fact that the static initializer has a stack map attribute in 1.6 that doesn't exist in 1.5. This also creates an offset in the references to constant pool entries. Hope this clarifies it.
(In reply to comment #4) > Hope this clarifies it. Yup. Thanks!
Released for 3.6M6 with changes reported in comment 2. Regression tests in: org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#testBug303448a org.eclipse.jdt.core.tests.compiler.regression.NullReferenceTest#testBug303448b And updated existing tests.
Verified for 3.6M6 using I20100307-2000.
