Community
Participate
Working Groups
As per http://dev.eclipse.org/mhonarc/lists/ide-dev/msg00368.html 2000+ Eclipse users made a statement towards letting the compiler report all potential programming problems by default. I suggest we make changes directly within JDT/Core rather than letting EPP modify the defaults, mostly for the sake of consistency. To serve our users best we should strike a balance between obeying to the survey result and applying some common sense (taking also into account the 34% No votes).
First question: what warnings exactly are we speaking about. Quoting John: "To be concrete, the survey talked about "Potential programming problems". In the Potential programming problems section, there are currently only the following set to ignore by default: Possible accidental boolean assignment e.g., if (a = b) Boxing and unboxing conversions Empty statement Unused object allocation Switch missing default case Switch case fall through Potential resource leak Missing synchronized modifier on inherited method Class overrides equals but not hashCode " So this is the time to validate if all warnings have been appropriately classified. E.g., we _could_ consider the groups "Generic types" and "Null analysis" as subgroups of "Potential programming problems". I'm not suggesting this, just mentioning. OTOH, as John mentions, the boxing/unboxing warnings are a bit of an outlier, could be reclassified under "Code style" (see also bug 163065). Also the "switch" warnings deserve a closer look. Despite having authored some of those, I wouldn't recommend all these warnings to everybody. See also the dedicated help page at http://help.eclipse.org/kepler/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Ftasks%2Ftask-ensuring_switch_completeness.htm&cp=1_3_9_2
On a different note, things would be a lot easier if bug 83548 would be fixed before resolving this bug, but that one definitely requires some cross-cutting coding which we cannot do before Java 8 has sailed.
(In reply to Stephan Herrmann from comment #1) > Potential resource leak Another one which I authored, but have hesitations about: this analysis still has a few known bugs, some of which may be around still in Luna. I'd be more comfortable to first improve the analysis and then enabled it by default. So, eventually, yes, I'm in favour of enabling by default, but I'm not sure when is the right point in time.
It makes perfect sense to go over John's list and change some of them to 'Warning' in JDT, but others are just not OK to be changed for everyone. Some Examples: - 'Class overrides equals but not hashCode' makes sense for most users, hence I'd agree to enable this. - There are many unused object allocations that are perfectly fine/desired, e.g. creating a new SWT Separator. Setting this to warning for everyone just sends the wrong message and there's currently no easy way to suppress this correct cases. - 'Possible accidental boolean assignment e.g., if (a = b)' is a valid coding pattern. Some like it some think it is bad. Not us to judge. - Boxing and unboxing conversions is not good to enable, because only one of them is unsafe. JDT should have separated those into two cases (see bug 163065 ) - At a first glance, missing default case probably also makes sense for most users. At any rate, I would not recommend EPP to override the JDT defaults.
(In reply to Dani Megert from comment #4) > - There are many unused object allocations that are perfectly fine/desired, > e.g. creating a new SWT Separator. Setting this to warning for everyone just > sends the wrong message and there's currently no easy way to suppress this > correct cases. This is actually on the list for which I'd love to flip the default :) Personally, I'd think this warning has one of the best ratios of valuable warnings vs. "false positives". As I'm not familiar with the SWT Separator, can you please explain what kind of use you see in 'unused' objects? Is it about constructors that make 'this' known to some other object, so the object will actually be connected after construction? Is that really a frequent coding style? If nothing else it establishes that 'this' is accessed before it's fully initialized - a potential programming problem in itself. Maybe we should just find a better way to suppress the rare useful creation of "unused" objects - i.e. better than putting @SW("unused") on the entire method.
I was planning on implementing the defaults in the EPP. I can also look into flipping the defaults in JDT but as you guys mentioned, this will also effect downstream products we don't know about. Should I hold off and wait for you guys to make a decision?
FWIW, if it's in EPP and it's now documented here who to blame!
(In reply to Stephan Herrmann from comment #5) > As I'm not familiar with the SWT Separator, can you please explain what kind > of use you see in 'unused' objects? Is it about constructors that make > 'this' known to some other object, so the object will actually be connected > after construction? Is that really a frequent coding style? Sorry, "Separator" should have been lower case, i.e., I didn't mean the 'Separator' class, but just how one usually adds a separator in the GUI, e.g.: new Label(composite, SWT.NONE); // separator For every widget that you create but where you don't need a reference afterwards, you will see this pattern. This happens often for SWT applications and probably also other frameworks. Another issue is that one cannot easily suppress it: - Since it's not assigned to a variable, one cannot suppress it locally. - The warning can be suppressed with the 'unused' token on the whole method, but that will hide other "unused" warnings that one might like to see.
(In reply to Gunnar Wagenknecht from comment #6) > I was planning on implementing the defaults in the EPP. I can also look into > flipping the defaults in JDT but as you guys mentioned, this will also > effect downstream products we don't know about. > > Should I hold off and wait for you guys to make a decision? We won't make a decision soon, since we're busy with Java 8 at least until end of March. (In reply to Gunnar Wagenknecht from comment #7) > FWIW, if it's in EPP and it's now documented here who to blame! I have trouble to parse that sentence ;-).
(In reply to Dani Megert from comment #8) > (In reply to Stephan Herrmann from comment #5) > > As I'm not familiar with the SWT Separator, can you please explain what kind > > of use you see in 'unused' objects? Is it about constructors that make > > 'this' known to some other object, so the object will actually be connected > > after construction? Is that really a frequent coding style? > > Sorry, "Separator" should have been lower case, i.e., I didn't mean the > 'Separator' class, but just how one usually adds a separator in the GUI, > e.g.: > > new Label(composite, SWT.NONE); // separator > > For every widget that you create but where you don't need a reference > afterwards, you will see this pattern. This happens often for SWT > applications and probably also other frameworks. Thanks, it's clearer now. This still goes against everything I personally learned and taught about OOP, and from here it would take more discussion whether this is still a potential (sic) programming problem (which I still believe it is) or more appropriately classified as a code style issue. For comparison: even creating a new thread is not effective until you invoke start(). Ideally, I would expect that constructors participating in this pattern be marked, e.g., with @SideEffect (and then I can defer detailed reasoning/recommendations to the documentation of that annotation :) ). You may also remember that initially this warning was implemented for exceptions only, in which case it is "always" (like: 9 nines) a bug. > Another issue is that one cannot easily suppress it: > - Since it's not assigned to a variable, one cannot suppress it locally. > - The warning can be suppressed with the 'unused' token on the whole method, > but that will hide other "unused" warnings that one might like to see. Two options: - Declaration site: @SideEffect - Usage site: //$SIDE-EFFECT$ In short: I'd love to iron out the rough edges for that comparatively tiny number of non-bug situations, so that the vast majority of classes / constructors can participate in this analysis by default -- after Java 8 that is ...
(In reply to Stephan Herrmann from comment #10) > > Sorry, "Separator" should have been lower case, i.e., I didn't mean the > > 'Separator' class, but just how one usually adds a separator in the GUI, > > e.g.: > > > > new Label(composite, SWT.NONE); // separator > > > > For every widget that you create but where you don't need a reference > > afterwards, you will see this pattern. This happens often for SWT > > applications and probably also other frameworks. > > Thanks, it's clearer now. > > This still goes against everything I personally learned and taught about > OOP, What definition of OOP says that this construct is not OK: new MyClass(someOtherObject); but after assigning to a variable/field it's OOP? > Ideally, I would expect that constructors participating in this pattern be > marked, e.g., with @SideEffect (and then I can defer detailed > reasoning/recommendations to the documentation of that annotation :) ). I don't agree that, e.g. new Label(composite, SWT.NONE) is called/having a side-effect - it simply creates a label in the given composite. We should improve our unused detection in the following way: Composite unusedComposite= new Composite(); new Label(unusedComposite, SWT.NONE); <== warn here Composite usedComposite= new Composite(); new Label(usedComposite, SWT.NONE); <== don't warn here usedComposite.doSomething(); At any rate, we should move the discussion about this concrete compiler option into its own bug.
(In reply to comment #1) > First question: what warnings exactly are we speaking about. > Quoting John: > > "To be concrete, the survey talked about "Potential programming problems". In > the Potential programming problems section, there are currently only the > following set to ignore by default: Speaking as one of the respondents, from the way the question was phrased I took it to mean all warnings, like is there a drop-down menu?-then it's included, even all those that I've changed to "ignore" myself, ALL of them. I definitely didn't consider or heaven-forbid crosscheck which items are in which warnings category before giving my answer.
(In reply to Dani Megert from comment #11) > At any rate, we should move the discussion about this concrete compiler > option into its own bug. Done: bug 441926.
