Fix Warnings

[wimjongman]

We have 130K warnings which are not good for the performance and image of the project.

Either:

• Suppress the warnings in the project preferences
• Fix them

[SteveSchafer-Innovent]

I'll work on these as I have time. If someone else wants to help we should agree on some kind of division of areas so we don't collide.

[hvbtup]

Regarding NLS-warnings: I had started with adding NON-NLS comments on individual lines in the Word and Excel emitters, but that is rather tedious work, wouldn't recommend that. I think it's better to switch those warning off for whole classes. Most of the Java classes in the engine do not contain translatable strings.

[SteveSchafer-Innovent]

Eclipse will automatically add the NON-NLS comments everywhere in quick-fix. That removes the tedious aspect. Would this then be a better solution? Alternately I can add @SuppressWarnings to each class or I can change the problem severity to "ignore" in org.eclipse.jtd.core.prefs in each project. The last one would have the least impact on the code.

[wimjongman]

No because it will also non-nls the text that has to be translated.

Switching off the error in the preferences would be the only quickfix

[hvbtup]

I still think that after skim reading a *.java source file (scanning for translatable messages) in most cases SuppressWarnings can be added to whole classes.

As a non-english developer and user I think that only texts inside the IDE should be translated, whereas all those text that only occur at runtime (debug, warning, info, error messages) should strictly not be translated.

The reasoning behind this is: If you are not a developer, then those texts are all gibberish for your anyway.

I know this from ~30 years. experience. Only in a handful of cases has any of our users understood the root cause of a stack trace.

Those messages are understandable only for developers. And even then, you often have to search the internet for more information. Since we don't have unique message ids (such like ORA-01403 for Oracle databases with the meaning "No data found" or "Keine Daten gefunden" in German), it would be counter-productive to have a e.g. a German translation. Developers are expected to understand a basic subset of English anyway, and when searching the internet, the chances to find some useful information are much higher with an english search phrase.

[wimjongman]

I do not agree with the first paragraph. The rest I do agree with.

[SteveSchafer-Innovent]

@wimjongman if you don't agree with suppressing warnings on whole classes then you wouldn't agree to changing the severity level to "ignore" across the board. Are we left with examining individual string constants?

[wimjongman]

If there is no plan to externalize translatable strings then I'm fine with changing the severity level. If the plan does come up in the future it is easy to bump the severity level back to a warning.

[SteveSchafer-Innovent]

I submitted a PR to change the severity level given that we can change it back in the future if necessary.

[SteveSchafer-Innovent]

@wimjongman did you close this intentionally? I only fixed a small fraction of the warnings.

Also: I submitted a new PR 834 to ignore jre compatibility issues and unused code issues.

[wimjongman]

Steve, it was closed automatically because of the linked PR that was merged. Thanks for reopening.

[SteveSchafer-Innovent]

In PR 834 wim suggested I use the source cleanup feature (preferences/java/code style/clean up) to fix some warnings. I found that when I selected most of the options, it reduced the total number of warnings from 81959 to 72005, a reduction of 9954 warnings. Most of those options probably didn't fix any warnings, but they did change a lot of files. The ones that I can tell fix warnings are Change non static accesses to static members using declaring type Change indirect accesses to static members to direct accesses (accesses through subtypes) Remove unused imports Remove unused private methods Remove unused private constructors Remove unused private types Remove unused private fields Remove unused local variables If I set just those it reduces the total number of warnings to 74643, a reduction of 7316. So some of the others are also fixing warnings but I can't tell which ones.

Even though a broad source cleanup only fixes a little over 10% of the warnings, should we do it anyway? (asking anyone who has an opinion)

[claesrosell]

I think that it would be great to fix 7316 warnings with "Clean Up"! It would take a long time to fix those manually.

[wimjongman]

Sure. Let's go! I'm in favor of an aggressive, but sensible source clean-up. With sensible I mean that there are some cleanup options that are questionable.

[SteveSchafer-Innovent]

Okay I'll start with the clean-up actions I listed to fix the 7316 warnings. After that we can look at additional clean-ups but I think it should be a separate issue.

[wimjongman]

The problem with multiple cleanup commits is the pollution it gives in the history.

[SteveSchafer-Innovent]

Well then we can do one massive clean-up, but which clean-up's do you think are questionable?

[wimjongman]

Steve, I have attached a very aggressive cleanup. Maybe we can go over it and remove anything we don't like. You can import it via the preferences

birtcleanup.xml

[SteveSchafer-Innovent]

I personally like "Remove redundant comparison statements" and "Remove unreachable block" in the Unnecessary Code section. The latter might fix a warning. I think many of the items in the Performance section are worth doing. I like "Pull up assignment" in Code Style because I think it improves readability. I personally am a fan of using 'final' modifiers wherever possible because it can catch more errors.

Everything else I agree with or don't care about.

[wimjongman]

I personally like "Remove redundant comparison statements" and "Remove unreachable block" in the Unnecessary Code section. The latter might fix a warning. I think many of the items in the Performance section are worth doing. I like "Pull up assignment" in Code Style because I think it improves readability.

These ones are fine with me

I personally am a fan of using 'final' modifiers wherever possible because it can catch more errors.

It is better but not used much. I can do without this one.

[SteveSchafer-Innovent]

I'm getting an unexpected refactoring error. Root exception: java.lang.IllegalArgumentException at org.eclipse.jdt.core.dom.LambdaExpression.setBody(LambdaExpression.java:284) at org.eclipse.jdt.internal.corext.fix.LambdaExpressionsFixCore$CreateLambdaOperation.rewriteAST(LambdaExpressionsFixCore.java:654) at org.eclipse.jdt.internal.corext.fix.CompilationUnitRewriteOperationsFixCore.createChange(CompilationUnitRewriteOperationsFixCore.java:98) at org.eclipse.jdt.internal.ui.fix.CleanUpFixWrapper.createChange(CleanUpFixWrapper.java:39) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring.calculateChange(CleanUpRefactoring.java:775) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring$CleanUpASTRequestor.calculateSolutions(CleanUpRefactoring.java:301) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring$CleanUpASTRequestor.acceptAST(CleanUpRefactoring.java:279) at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:930) at org.eclipse.jdt.core.dom.CompilationUnitResolver.resolve(CompilationUnitResolver.java:614) at org.eclipse.jdt.core.dom.ASTParser.createASTs(ASTParser.java:954) at org.eclipse.jdt.internal.corext.dom.ASTBatchParser.createASTs(ASTBatchParser.java:82) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring$CleanUpFixpointIterator.next(CleanUpRefactoring.java:399) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring.cleanUpProject(CleanUpRefactoring.java:682) at org.eclipse.jdt.internal.corext.fix.CleanUpRefactoring.checkFinalConditions(CleanUpRefactoring.java:642) at org.eclipse.ltk.core.refactoring.CheckConditionsOperation.run(CheckConditionsOperation.java:86) at org.eclipse.ltk.core.refactoring.CreateChangeOperation.run(CreateChangeOperation.java:122) at org.eclipse.ltk.core.refactoring.PerformChangeOperation.run(PerformChangeOperation.java:210) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2313) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2338) at org.eclipse.ltk.internal.ui.refactoring.WorkbenchRunnableAdapter.run(WorkbenchRunnableAdapter.java:89) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:122) This was caused by "Convert functional interface instances" in "Java Feature" After deselecting that clean-up it succeeded but now there are 43 errors in the code

It seems some of these clean-ups are less then perfect. For example, it converted fontAttribs.put(TextAttribute.SIZE, new Float(fd.getSize() * getDpiResolution() / 72d)); to fontAttribs.put(TextAttribute.SIZE, Float.valueOf((float) fd.getSize() * getDpiResolution() / 72d)); but since it didn't add parentheses, the cast didn't work and the value passed to valueOf was a double.

In another instance it converted an inner class to static but didn't change the instantiation so it was still being instantiated as an instance member of the parent object.

I'm going to have to disable clean-ups until I can eliminate all the errors.

[wimjongman]

Oops, that is not very reassuring for the code that it did convert. I have filed https://bugs.eclipse.org/bugs/show_bug.cgi?id=578880

[SteveSchafer-Innovent]

I'll determine which clean-ups cause which errors and post the info to your bug.

[SteveSchafer-Innovent]

I've been able to eliminate the clean-up actions that were causing errors and will submit a PR.

Convert control statement bodies to block Combine nested 'if' statement in 'else' block to 'else if' Simplify lambda expression and method reference syntax Pull up assignment Use instanceof keyword instead of Class.isInstance() Exit loop earlier Replace String concatenation by StringBuilder Convert StringBuffer to StringBuilder for local variables Use String.replace() instead of String.replaceAll() when possible String.isBlank() rather than String.strip().isEmpty() Use lazy logical operator (&& and ||) Primitive comparison Primitive parsing Primitive serialization Primitive type rather then wrapper class Precompile reused regular expressions Remove unnecessary String creation Use boolean literals instead of Boolean.TRUE/FALSE when used as a primitive Remove unused imports Remove unused private methods Remove unused private constructors Remove unused private types Remove unused private fields Add missing '@Override' annotations Add missing '@Override' annotations to implementations of interface methods Add missing '@Deprecated' annotations Remove redundant modifiers Remove redundant semicolons Create array with curly Remove useless return Remove useless continue Convert loop into if Remove unnecessary '$NON-NLS$' tags Organize imports Format source code Remove trailing white spaces on all lines Remove redundant String.substring() parameter Use Arrays.fill() when possible Evaluate without null check Boolean value rather than comparison Avoid double negation Remove overridden assignment Remove redundant comparison statement Remove unreachable block Operate on Maps directly Initialize collection at creation Initialize map at creation Factorize operands Single 'if' statement rather than duplicate blocks that fall through Remove redundant if condition Use Comparator.comparing() Use try-with-resource Multi-catch Use Java method instead of system property 'FILE_ENCODING' Use Java method instead of system property 'PATH_SEPARATOR' Use Java method instead of system property 'FILE_SEPARATOR' Use Java method instead of system property 'LINE_SEPARATOR' Use Java method instead of system property 'BOOLEAN_PROPERTY' Use diamond operator Use Objects.hash() Use Objects.equals() in the equals method implementation Use Autoboxing

[SteveSchafer-Innovent]

Looks like the commit by Claes "Replace iText with OpenPDF #767 (#814)" caused 612 errors in eclipse, mostly because it can't find com.lowagie and other PDF related errors. Any suggestions how I can remove these errors?

[claesrosell]

@SteveSchafer-Innovent, try to reload your target platform.

[claesrosell]

On the theme "less then perfect clean-ups", this is the reason why #836 currently fails : The "Replace String concatenation by StringBuilder" clean up converts

	public String getDisplayLabel(Module module, int level) {
		String displayLabel = super.getDisplayLabel(module, level);
		if (level == IDesignElementModel.FULL_LABEL) {
			String name = getStringProperty(module, IReportItemModel.DATA_SET_PROP);
			name = limitStringLength(name);
			if (!StringUtil.isBlank(name)) {
				displayLabel += "(" + name + ")"; //$NON-NLS-1$//$NON-NLS-2$
			}
		}
		return displayLabel;
	}

to

	@Override
	public String getDisplayLabel(Module module, int level) {
		StringBuilder displayLabel = new StringBuilder().append(super.getDisplayLabel(module, level));
		if (level == IDesignElementModel.FULL_LABEL) {
			String name = getStringProperty(module, IReportItemModel.DATA_SET_PROP);
			name = limitStringLength(name);
			if (!StringUtil.isBlank(name)) {
				displayLabel.append("(").append(name).append(")"); //$NON-NLS-1$//$NON-NLS-2$
			}
		}
		return displayLabel.toString();
	}

The old implementation will return null if super.getDisplayLabel() returns null and level != IDesignElementModel.FULL_LABEL , and the new implementation will return "null" under the same circumstances.

This is of cause suspicious code to begin with since the original code will throw a NPE if super.getDisplayLabel() returns null and level == IDesignElementModel.FULL_LABEL. but it is not a good sign that the clean up changes the behavior of the method.

I was initially very optimistic about these "automatic clean-ups" but I am starting to get scared instead.

[SteveSchafer-Innovent]

Not done yet.

[SteveSchafer-Innovent]

Are there reasons not to move all Bundle-RequiredExecutionEnvironment's to JavaSE-11?

[wimjongman]

I can't think of a reason.