Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 504473

Summary:	Fix build warnings in I20161005-1430
Product:	[Eclipse Project] JDT	Reporter:	Stefan Xenos <sxenos>
Component:	Core	Assignee:	Stefan Xenos <sxenos>
Status:	VERIFIED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	daniel_megert, jarthana, manoj.palat, stephan.herrmann
Version:	4.6	Flags:	manoj.palat: review-
Target Milestone:	4.7 M3
Hardware:	PC
OS:	Linux
See Also:	https://git.eclipse.org/r/82572 https://git.eclipse.org/r/82985 https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=888efa82e8bf7dca948fafeddbba604ce0d35d22
Whiteboard:

Description Stefan Xenos

2016-10-06 00:13:50 EDT

These warnings only show up in builds, not in the workspace ( for example, http://download.eclipse.org/eclipse/downloads/drops4/I20161005-1430/ )

Representative error:

/search/org/eclipse/jdt/internal/core/nd/java/NdAnnotationInVariable.java : 1 warning :

OTHER WARNINGS
1. WARNING in /search/org/eclipse/jdt/internal/core/nd/java/NdAnnotationInVariable.java
 (at line 20)
@SuppressWarnings("hiding")
Unnecessary @SuppressWarnings("hiding")

It looks like the Hudson builds are ignoring the fact that JDT core enables the "hiding" warning, so it generates warnings when it is unnecessarily suppressed.

However, since the new indexer code intentionally hides the "type" variable in almost every class, it probably makes sense to just disable this warning in the workspace so that it both matches Hudson, removes the warnings, and doesn't require the extra @SuppressWarnings tags.

Comment 1 Eclipse Genie

2016-10-06 00:19:13 EDT

New Gerrit change created: https://git.eclipse.org/r/82572

Comment 2 Dani Megert

2016-10-06 04:04:25 EDT

(In reply to Stefan Xenos from comment #0)
> It looks like the Hudson builds are ignoring the fact that JDT core enables
> the "hiding" warning, so it generates warnings when it is unnecessarily
> suppressed.

Any build (also PDE) uses the defaults and not the project settings. You can tell PDE (in build.properties) and Hudson (pom.xml) to enable the "hiding" warning.

Take a look at how jdt.ui disables unavoidableGenericProblems.

NOTE: Before enabling it in the build, make sure that in the code all problems are fixed (set it to error to test).

Comment 3 Dani Megert

2016-10-06 05:19:58 EDT

(In reply to Dani Megert from comment #2)
> (In reply to Stefan Xenos from comment #0)
> > It looks like the Hudson builds are ignoring the fact that JDT core enables
> > the "hiding" warning, so it generates warnings when it is unnecessarily
> > suppressed.
> 
> Any build (also PDE) uses the defaults and not the project settings.

Just to clarify: it uses the defaults unless overridden in the top-level parent POM.

Comment 4 Stephan Herrmann

2016-10-06 06:27:43 EDT

Seeing this in the proposed change:
  org.eclipse.jdt.core.compiler.problem.fieldHiding=ignore
I believe it's good to have a consensus about such changes before pushing.

In this particular case I kind-of agree *because* we already require all field references to be qualified with "this". In this context, allowing locals to hide a field has two implications:
+ our accustomed style of coding avoids ambiguity between local and field
- we no longer get a warning when we forget "this.".

What do others think?

Comment 5 Dani Megert

2016-10-11 03:20:50 EDT

Ping!

Comment 6 Stefan Xenos

2016-10-11 10:49:44 EDT

Waiting for project lead approval before pushing this change.

Comment 7 Jay Arthanareeswaran

2016-10-12 01:26:36 EDT

(In reply to Stephan Herrmann from comment #4)
> - we no longer get a warning when we forget "this.".

I have found this useful in the past. In effect, we will be ignoring two warnings with this change. I don't mind introducing new warning to be in line with the global SDK build settings, but don't want to let go of existing ones. So, I will go with Dani's suggestion of adding custom warning options:

    <code.ignoredWarnings>-warn:-fieldHiding</code.ignoredWarnings>

to the pom.xml and

javacWarnings..=-fieldHiding to the build properties.

Comment 8 Manoj N Palat

2016-10-12 01:30:40 EDT

(In reply to Jay Arthanareeswaran from comment #7)
> (In reply to Stephan Herrmann from comment #4)
> > - we no longer get a warning when we forget "this.".
> 
> I have found this useful in the past. In effect, we will be ignoring two
> warnings with this change. I don't mind introducing new warning to be in
> line with the global SDK build settings, but don't want to let go of
> existing ones. So, I will go with Dani's suggestion of adding custom warning
> options:
> 
>     <code.ignoredWarnings>-warn:-fieldHiding</code.ignoredWarnings>
> 
> to the pom.xml and
> 
> javacWarnings..=-fieldHiding to the build properties.

I also wouldn't want to let go of any existing warnings. 
+1 for Jay's suggestion.

Comment 9 Eclipse Genie

2016-10-12 01:42:22 EDT

New Gerrit change created: https://git.eclipse.org/r/82984

Comment 10 Eclipse Genie

2016-10-12 02:01:28 EDT

New Gerrit change created: https://git.eclipse.org/r/82985

Comment 11 Jay Arthanareeswaran

2016-10-12 02:15:12 EDT

(In reply to Eclipse Genie from comment #10)
> New Gerrit change created: https://git.eclipse.org/r/82985

This one is bad too. Sorry about the noise, will fix this.

Comment 12 Eclipse Genie

2016-10-12 15:18:02 EDT

Gerrit change https://git.eclipse.org/r/82985 was merged to [master].
Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=888efa82e8bf7dca948fafeddbba604ce0d35d22

Comment 13 Jay Arthanareeswaran

2016-10-13 00:10:48 EDT

(In reply to Eclipse Genie from comment #12)
> Gerrit change https://git.eclipse.org/r/82985 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/
> ?id=888efa82e8bf7dca948fafeddbba604ce0d35d22

Stefan, not sure if my changes were correct. I saw warnings in the Hudson build hence aborted. I meant to fix it today. Or am I missing something?

Comment 14 Dani Megert

2016-10-13 04:08:51 EDT

The problem is still there in the lasted build. And this is expected. '-' disables a diagnostic - you need to use '+' ;-)

Comment 15 Jay Arthanareeswaran

2016-10-13 05:19:14 EDT

I have fixed it now:

http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=a4212f660b3eb77ae0a548ba05f5445b9b81d72c

Also verified with a local build that there are no warnings. Not really sure how to verify the changes in the build.properties, though.

Comment 16 Dani Megert

2016-10-14 03:57:53 EDT

Verified in N20161013-2000.