Community
Participate
Working Groups
I20081119-1600 Follow-up to bug 48399. After enabling the new dead code detection compiler option, we found that most hits in our codebase were debug switches as exemplified in JLS3 13.4.9 and 14.21: class Flags { final static boolean DEBUG = true; } class Test { public static void main(String[] args) { if (Flags.DEBUG) System.out.println("debug is true"); } } Could you add this as a sub-option of COMPILER_PB_DEAD_CODE? I would only expect this exception rule to suppress warnings of one of the forms ... - 'if (DEBUG) THEN' or - 'if (DEBUG) THEN else ELSE' ... where ... - DEBUG is the qualified or unqualified name of a 'static final boolean' field which is initialized to either 'true' or 'false', and - either THEN or ELSE is a statement that is marked as dead code.
What about ? if (DEBUG_LEVEL == 1) { ... }
> if (DEBUG_LEVEL == 1) { > ... > } My guess is that this would lower the rate of remaining false positives after comment 0 from 1% to 0.9%. But the drawback would be that there's no easy rule to describe this condition. E.g. the rule "suppress the problem if the condition contains a reference to a static final field which is initialized by a literal" would also suppress if (MyEvent.CHANGED == MyEvent.NEW) { ... } or if (0 == MyEvent.NEW) { ... } , although this was actually meant to be 'if (event.type == MyEvent.NEW)'.
>What about ? > >if (DEBUG_LEVEL == 1) { > ... >} I know that there are more patterns beside the one from comment 0. The question is which ones are common. I did a scan in the SDK and found that lots of plug-ins use the pattern from comment 0 while I found almost no DEBUG_LEVEL case. Note that I'm not against also supporting additional patterns, but I don't buy the argument that only because there's yet another case we can't satisfy the 90% (or higher) case. Just FYI: when I enabled the problem in my workspace I got 330 dead code problems reported, out of which I found only 17 real issues and probably missed some due to the big amount of false positives.
Don't get me wrong. I am willing to add support for a good heuristic. I am only trying to come up with a reasonable good one where we cannot be blamed to be to nice to ourselves. What about these cases: (1) if (!TRACE_DISABLED) System.out.println("fake reachable1"); ---> I think we should tolerate, as another known pattern. (2) if (DEBUG && b) System.out.println("fake reachable2"); --> presently, it complains both against 'b' and thenStatement --> I would keep that, since not a simple pattern. --> argue that one could write instead: if (DEBUG) { if (b) { ... } } (3) if (DEBUG && check()) System.out.println("fake reachable3"); --> presently, it complains both against 'check()' and thenStatement --> I would keep that, since not a simple pattern. --> argue that one could write instead: if (DEBUG) { if (check()) { ... } } (4) if (b && DEBUG) System.out.println("fake reachable4"); ---> would complain, I don't think anyone write such code (i.e. DEBUG check goes first) (5) if (check() && DEBUG) System.out.println("fake reachable5"); ---> would complain, I don't think anyone write such code (i.e. DEBUG check goes first) (6) if (DEBUG_LEVEL > 1) System.out.println("fake reachable6"); //$NON-NLS-1$ ---> I think we should tolerate, as another known pattern.
Agree with comment 4. I would add two sub-options: [ ] Ignore if-test on boolean constant (e.g. if (DEBUG) ...) [ ] Ignore if-test on number constant (e.g. if (DEBUG_LEVEL > 1) Case 6) could also handle floats and double I guess.
Does it really need 2 sub prefs ? [ ] Ignore trivial if check on constant (e.g. if (DEBUG), if (DEBUG_LEVEL > 1) (and yes, the binary expression could tolerate any sort of constants in there, like if (DEBUG == true) ...
>Does it really need 2 sub prefs ? Yes, because for people (90% case) don't use the second pattern and if they have code like: SOME_CONST > 1 they would like this to be flagged.
The consensus here (Jerome, David, Frederic and I) is to go for one sub pref for trivial checks.
Too bad JDT UI are only two people so we have no chance when it comes to voting :-( The UI would be much clearer as we could explain the two cases - and as said, 90% don't use the second pattern. Just do a scan of the SDK once.
> The consensus here (Jerome, David, Frederic and I) is to go for one sub pref > for trivial checks. +1 for only one sub-pref, but only for trivial *boolean* checks.
>+1 for only one sub-pref, but only for trivial *boolean* checks. That would also be my vote unless we find that the other case is really common and a pain for users.
Triviality can be argued further, I agree. I can turn off the "if (DEBUG_LEVEL > 0)" scenario to see if this becomes a popular request to add.
I would even enable dead code detection by default, but only with 'Ignore if-test on boolean constant' enabled as well. Even if we had the more elaborate option for number checks, I would not enable that one by default (since it could wrongly hide real issues, which I think is not the case with only the boolean checks enabled).
Just for the records: I loaded all SDK source into a fresh workspace and did the following two case sensitive regex searches: 1) if.*\(\s*DEBUG.*\) ==> 1315 matches (including the ones below) 2) if.*\(\s*DEBUG.*[<>].*\) ==> 2 matches (1 in o.e.jdt.core and 1 in o.e.osgi)
Created attachment 118822 [details] Proposed patch Only tolerates: if(<ref to constant>) and if(!<ref to constant>)
Released for 3.5M4. Fixed If triviality needs to be argued further, please enter a separate bug report.
Perfect! Thanks for the quick fix. When enabled on my plug-ins, it found exactly those locations where a potential problem is. BTW: I would follow Markus's suggestion and set this to warning by default.
Verified for 3.5M4 using I20081209-0100
*** Bug 313514 has been marked as a duplicate of this bug. ***
