Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 17:03:54 GMT, Andy Goryachev wrote: > This comment might be out of place, but since we started fixing raw type use > may be you could take a look at it? I know it's in javafx.graphics, so not > technically a part of this PR, but It's pretty weird. Bringing it up here > because it is relevant to the overall discussion. Raw type fixing is not part of this PR, I would like to do that separately as it will be more involved (when fixing a raw type, usually other warnings pop up, like unnecessary casts or unchecked warnings). > If I modify CssParser (any small change would do, even insert a space), > Eclipse suddenly gives me a bunch of errors. I can reproduce this. This looks like a bug in the Eclipse compiler. The Eclipse Compiler is incremental and may not be recompiling the correct sources affected, but on a full rebuild it does. It should probably be reported if it can be narrowed down a bit. - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 20:43:11 GMT, John Hendrikx wrote: >> I didn't fix all problems, only the easy non-controversial ones. Things >> that might actually be bugs require more careful review and so are best >> saved for smaller PR's IMHO. >> >> This is already fixing around 2000 warnings in 250 files, and before going >> in even deeper I'd like to see if this will actually get accepted into >> JavaFX in a reasonable time frame (PR's changing large numbers of files >> don't age well). > > I only fixed the warnings listed in the ticket :) but the ticket specifies (javafx.)base, right? - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 20:46:57 GMT, Andy Goryachev wrote: >> I only fixed the warnings listed in the ticket :) > > but the ticket specifies (javafx.)base, right? let's create a followup ticket - the messed up statements in Disposer and especially in PrismFontFactory:1401 are worrying. cc: @prrace - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 11:53:30 GMT, John Hendrikx wrote: > - Remove unsupported/unnecessary SuppressWarning annotations > - Remove reduntant type specifications (use diamond operator) > - Remove unused or duplicate imports > - Remove unnecessary casts (type is already correct type or can be autoboxed) > - Remove unnecessary semi-colons (at end of class definitions, or just > repeated ones) > - Remove redundant super interfaces (interface that is already inherited) > - Remove unused type parameters > - Remove declared checked exceptions that are never thrown > - Add missing `@Override` annotations I am not disputing the benefits of generics, but it does have its limits (see many typecasts in TableView and that CssParser example I provided above). Especially CssParser, because it produces errors in one case, but not in the other. And I agree - perhaps unused imports and missing overrides should be checked by the tool chain - since it already bothers with trailing whitespace. - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 20:27:27 GMT, Andy Goryachev wrote: >> full list >> >> >> Description ResourceLocation >> Empty control-flow statement DateCellBehavior.java line 95 >> Empty control-flow statement Disposer.java line 65 >> Empty control-flow statement Disposer.java line 66 >> Empty control-flow statement Disposer.java line 66 >> Empty control-flow statement Disposer.java line 72 >> Empty control-flow statement DualPivotQuicksort20191112Ext.java line 267 >> Empty control-flow statement DualPivotQuicksort20191112Ext.java line 268 >> Empty control-flow statement DualPivotQuicksort20191112Ext.java line 380 >> Empty control-flow statement DualPivotQuicksort20191112Ext.java line 494 >> Empty control-flow statement DualPivotQuicksort20191112Ext.java line 673 >> Empty control-flow statement DualPivotQuicksort20191112Ext.java line 678 >> Empty control-flow statement DualPivotQuicksort20191112Ext.java line 686 >> Empty control-flow statement DualPivotQuicksort20191112Ext.java line 800 >> Empty control-flow statement DualPivotQuicksort20191112Ext.java line 808 >> Empty control-flow statement KeyboardShortcutsHandler.java line 498 >> Empty control-flow statement PrismFontFactory.java line 1401 >> Unnecessary semicolonAbstractPrimaryTimerTest.java line 188 >> Unnecessary semicolonActivity.java line 31 >> Unnecessary semicolonAnimationMock.java line 41 >> Unnecessary semicolonArc2D.java line 388 >> Unnecessary semicolonBaseMesh.java line 68 >> Unnecessary semicolonBindingsNumberCalculationsTest.java line 838 >> Unnecessary semicolonBloom.java line 90 >> Unnecessary semicolonBoxBlur.javaline 95 >> Unnecessary semicolonButtonBaseTest.java line 182 >> Unnecessary semicolonCachingShapeRep.javaline 234 >> Unnecessary semicolonCascadingStyle.java line 184 >> Unnecessary semicolonCellUtils.java line 155 >> Unnecessary semicolonCellUtils.java line 305 >> Unnecessary semicolonChoiceBoxTreeCell.java line 340 >> Unnecessary semicolonClipboard.java line 389 >> Unnecessary semicolonClipboardContent.java line 212 >> Unnecessary semicolonColorAdjust.javaline 98 >> Unnecessary semicolonColorInput.java line 83 >> Unnecessary semicolonColorPalette.java line 445 >> Unnecessary semicolonComboBoxTreeCell.java line 373 >> Unnecessary semicolonContextMenuTest.javaline 259 >> Unnecessary semicolonCssParser.java line 2493 >> Unnecessary semicolonCssParser.java line 3830 >> Unnecessary semicolonCssParser.java line 3834 >> Unnecessary semicolonD3DFrameStats.java line 54 >> Unnecessary semicolonDefaultCancelButtonTestBase.javaline 304 >> Unnecessary semicolonDisplacementMap.javaline 108 >> Unnecessary semicolonDisplacementMap.javaline 243 >> Unnecessary semicolonDropShadow.java line 148 >> Unnecessary semicolonDummyTexture.java line 41 >> Unnecessary semicolonDumpRenderTree.java line 94 >> Unnecessary semicolonDumpRenderTree.java line 106 >> Unnecessary semicolonFXCanvas.java line 171 >> Unnecessary semicolonFXCanvas.java line 1254 >> Unnecessary semicolonFXDnDInteropN.java line 374 >> Unnecessary semicolonFireButtonBaseTest.java line 92 >> Unnecessary semicolonGaussianBlur.java line 83 >> Unnecessary semicolonGlow.java line 82 >> Unnecessary semicolonHyperlinkTest.java line 250 >> Unnecessary semicolonImage.java line 248 >> Unnecessary semicolonImageInput.java line 81 >> Unnecessary semicolonImageStorage.java line 115 >> Unnecessary semicolonInnerShadow.javaline 128 >> Unnecessary semicolonIosImageLoaderFactory.java line 42 >> Unnecessary semicolonJ2DPrinter.java line 865 >> Unnecessary semicolonJ2DPrinterJob.java line 776 >> Unnecessary semicolonKeyCode.javaline 1177 >> Unnecessary semicolonKeyCodeMap.java line 62 >> Unnecessary semicolonKeyFrameTest.java line 131 >> Unnecessary semicolonKeyFrameTest.java line 136 >> Unnecessary semicolonKeyFrameTest.java line 141 >> Unnecessary semicolonKeyFrameTest.java line 146 >> Unnecessary semicolonKeyFrameTest.java line 218 >> Unnecessary semicolonKeyFrameTest.java line 223 >> Unnecessary semicolonKeyFrameTest.java line 228 >> Unnecessary semicolonLeakTest.java line 187 >> Unnecessary semicolonLeakTest.java line 348 >> Unnecessary semicolonLeakTest.java line 474 >> Unnecessary semicolonLeakTest.java line 205 >> Unnecessary semicolonLeakTest.java lin
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 20:42:51 GMT, John Hendrikx wrote: >> many of those ^^ appear to be bugs. > > I didn't fix all problems, only the easy non-controversial ones. Things that > might actually be bugs require more careful review and so are best saved for > smaller PR's IMHO. > > This is already fixing around 2000 warnings in 250 files, and before going in > even deeper I'd like to see if this will actually get accepted into JavaFX in > a reasonable time frame (PR's changing large numbers of files don't age well). I only fixed the warnings listed in the ticket :) - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 16:57:30 GMT, Andy Goryachev wrote: > One suggestion I'd like to make is to publish the Compiler Errors/Warnings > configuration for Eclipse. I have a set of screenshots that produce a 0 > err/warn count with the current master. Eclipse also seem to provide a way to > export/import these configs, but for some reason export/import stopped > working for me around a decade ago. > > I also noticed two things in general: > > 1. the default err/warn configuration enables meaningless warnings and > disables those that point to the real problems (see > [JDK-8289379](https://bugs.openjdk.org/browse/JDK-8289379)) The default configuration of Eclipse is a very conservative starting point, and shouldn't really be used for anything serious about code quality. Also, many of the warnings are not unique to Eclipse (it's just that Eclipse users notice them far easier thanks to a global problem view). The raw type warnings, unchecked casts and many others are part of `javac` as well. > 2. in large code bases with multiple contributors, it makes little sense to > enable warnings like usage of raw type or unused imports since they a) don't > contribute to code quality and b) are getting re-introduced all the time by > people who don't use Eclipse I disagree, raw types warnings are of great value. Raw types were used in the pre Java 1.5 days, and `ClassCastException`s were a big problem then. After Java got generics, many of these can now be checked by the compiler and will result in compile errors. List list = new ArrayList(); list.add(2); String s = (String)list.get(0); The above code will have 0 warnings apart from raw type usage. It is also obviously wrong and will result in a runtime exception. This wouldn't compile if the list was typed. Unused imports are less of a problem I agree, but some hygiene there can help. For example, importing things just because Javadoc refers to it can point to problems in documentation where higher level concepts are referred to from a lower level abstraction. In projects I usually work on, we tend to have a fixed import order to avoid diffs on imports. Any diffs on imports then more clearly show you're adding/removing new dependencies -- depending on the code involved this can already be insightful to see if the code isn't doing too much or doing things it shouldn't be doing (ie. importing `java.sql.Date` instead of `java.util.Date` -- something you won't notice when only looking at the code). However, the best way to handle keeping a code base clean with many contributors is to make it part of the build process; the first step to achieve this is to clean the code base. Once the code is nearly warning free, the build can enforce this by compiling with linting on and failing the build if there are warnings (like for Javadoc). I think the Skara tooling might be able to reject PR's with warnings, if not, Gradle can fail builds with warnings (but that's a bit more heavy handed). I've attached the Eclipse preferences that I've been using (please remove the txt extension that I only added to allow pasting it in this comment). [javafx.epf.txt](https://github.com/openjdk/jfx/files/10060251/javafx.epf.txt) - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 20:26:54 GMT, Andy Goryachev wrote: >> Cool. My eclipse finds other places which do look like a bug - see >> disposed:65 >> It is in the base - why isn't it this PR? > > full list > > > Description ResourceLocation > Empty control-flow statement DateCellBehavior.java line 95 > Empty control-flow statement Disposer.java line 65 > Empty control-flow statement Disposer.java line 66 > Empty control-flow statement Disposer.java line 66 > Empty control-flow statement Disposer.java line 72 > Empty control-flow statement DualPivotQuicksort20191112Ext.java line 267 > Empty control-flow statement DualPivotQuicksort20191112Ext.java line 268 > Empty control-flow statement DualPivotQuicksort20191112Ext.java line 380 > Empty control-flow statement DualPivotQuicksort20191112Ext.java line 494 > Empty control-flow statement DualPivotQuicksort20191112Ext.java line 673 > Empty control-flow statement DualPivotQuicksort20191112Ext.java line 678 > Empty control-flow statement DualPivotQuicksort20191112Ext.java line 686 > Empty control-flow statement DualPivotQuicksort20191112Ext.java line 800 > Empty control-flow statement DualPivotQuicksort20191112Ext.java line 808 > Empty control-flow statement KeyboardShortcutsHandler.java line 498 > Empty control-flow statement PrismFontFactory.java line 1401 > Unnecessary semicolon AbstractPrimaryTimerTest.java line 188 > Unnecessary semicolon Activity.java line 31 > Unnecessary semicolon AnimationMock.java line 41 > Unnecessary semicolon Arc2D.java line 388 > Unnecessary semicolon BaseMesh.java line 68 > Unnecessary semicolon BindingsNumberCalculationsTest.java line 838 > Unnecessary semicolon Bloom.java line 90 > Unnecessary semicolon BoxBlur.javaline 95 > Unnecessary semicolon ButtonBaseTest.java line 182 > Unnecessary semicolon CachingShapeRep.javaline 234 > Unnecessary semicolon CascadingStyle.java line 184 > Unnecessary semicolon CellUtils.java line 155 > Unnecessary semicolon CellUtils.java line 305 > Unnecessary semicolon ChoiceBoxTreeCell.java line 340 > Unnecessary semicolon Clipboard.java line 389 > Unnecessary semicolon ClipboardContent.java line 212 > Unnecessary semicolon ColorAdjust.javaline 98 > Unnecessary semicolon ColorInput.java line 83 > Unnecessary semicolon ColorPalette.java line 445 > Unnecessary semicolon ComboBoxTreeCell.java line 373 > Unnecessary semicolon ContextMenuTest.javaline 259 > Unnecessary semicolon CssParser.java line 2493 > Unnecessary semicolon CssParser.java line 3830 > Unnecessary semicolon CssParser.java line 3834 > Unnecessary semicolon D3DFrameStats.java line 54 > Unnecessary semicolon DefaultCancelButtonTestBase.javaline 304 > Unnecessary semicolon DisplacementMap.javaline 108 > Unnecessary semicolon DisplacementMap.javaline 243 > Unnecessary semicolon DropShadow.java line 148 > Unnecessary semicolon DummyTexture.java line 41 > Unnecessary semicolon DumpRenderTree.java line 94 > Unnecessary semicolon DumpRenderTree.java line 106 > Unnecessary semicolon FXCanvas.java line 171 > Unnecessary semicolon FXCanvas.java line 1254 > Unnecessary semicolon FXDnDInteropN.java line 374 > Unnecessary semicolon FireButtonBaseTest.java line 92 > Unnecessary semicolon GaussianBlur.java line 83 > Unnecessary semicolon Glow.java line 82 > Unnecessary semicolon HyperlinkTest.java line 250 > Unnecessary semicolon Image.java line 248 > Unnecessary semicolon ImageInput.java line 81 > Unnecessary semicolon ImageStorage.java line 115 > Unnecessary semicolon InnerShadow.javaline 128 > Unnecessary semicolon IosImageLoaderFactory.java line 42 > Unnecessary semicolon J2DPrinter.java line 865 > Unnecessary semicolon J2DPrinterJob.java line 776 > Unnecessary semicolon KeyCode.javaline 1177 > Unnecessary semicolon KeyCodeMap.java line 62 > Unnecessary semicolon KeyFrameTest.java line 131 > Unnecessary semicolon KeyFrameTest.java line 136 > Unnecessary semicolon KeyFrameTest.java line 141 > Unnecessary semicolon KeyFrameTest.java line 146 > Unnecessary semicolon KeyFrameTest.java line 218 > Unnecessary semicolon KeyFrameTest.java line 223 > Unnecessary semicolon KeyFrameTest.java line 228 > Unnecessary semicolon LeakTest.java line 187 > Unnecessary semicolon LeakTest.java line 348 > Unnecessary semicolon LeakTest.java line 474 > Unnecessary semicolon LeakTest.java line 205 > Unnecessary semicolon LeakTest.java line 210 > Unnecessary semicolon LeakTest.java line 215 > Unnecessary semicolon LightBaseTest.java line 89 > Unnecessary semicolon Lighting.java line 77 > Unnecessary semicolon Lighting.java line 164 > Unnecessary semicolon LineChartTest.java line 55 > Unnecessary semicolon LinearConvolveRenderState.java line 124 > Unnecessary semico
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 20:25:06 GMT, Andy Goryachev wrote: >> Yes, `potential programming problems / empty statement`. It can sometimes >> point to a mistake where a semi colon is used just before a block. > > Cool. My eclipse finds other places which do look like a bug - see > disposed:65 > It is in the base - why isn't it this PR? full list Description ResourceLocation Empty control-flow statementDateCellBehavior.java line 95 Empty control-flow statementDisposer.java line 65 Empty control-flow statementDisposer.java line 66 Empty control-flow statementDisposer.java line 66 Empty control-flow statementDisposer.java line 72 Empty control-flow statementDualPivotQuicksort20191112Ext.java line 267 Empty control-flow statementDualPivotQuicksort20191112Ext.java line 268 Empty control-flow statementDualPivotQuicksort20191112Ext.java line 380 Empty control-flow statementDualPivotQuicksort20191112Ext.java line 494 Empty control-flow statementDualPivotQuicksort20191112Ext.java line 673 Empty control-flow statementDualPivotQuicksort20191112Ext.java line 678 Empty control-flow statementDualPivotQuicksort20191112Ext.java line 686 Empty control-flow statementDualPivotQuicksort20191112Ext.java line 800 Empty control-flow statementDualPivotQuicksort20191112Ext.java line 808 Empty control-flow statementKeyboardShortcutsHandler.java line 498 Empty control-flow statementPrismFontFactory.java line 1401 Unnecessary semicolon AbstractPrimaryTimerTest.java line 188 Unnecessary semicolon Activity.java line 31 Unnecessary semicolon AnimationMock.java line 41 Unnecessary semicolon Arc2D.java line 388 Unnecessary semicolon BaseMesh.java line 68 Unnecessary semicolon BindingsNumberCalculationsTest.java line 838 Unnecessary semicolon Bloom.java line 90 Unnecessary semicolon BoxBlur.javaline 95 Unnecessary semicolon ButtonBaseTest.java line 182 Unnecessary semicolon CachingShapeRep.javaline 234 Unnecessary semicolon CascadingStyle.java line 184 Unnecessary semicolon CellUtils.java line 155 Unnecessary semicolon CellUtils.java line 305 Unnecessary semicolon ChoiceBoxTreeCell.java line 340 Unnecessary semicolon Clipboard.java line 389 Unnecessary semicolon ClipboardContent.java line 212 Unnecessary semicolon ColorAdjust.javaline 98 Unnecessary semicolon ColorInput.java line 83 Unnecessary semicolon ColorPalette.java line 445 Unnecessary semicolon ComboBoxTreeCell.java line 373 Unnecessary semicolon ContextMenuTest.javaline 259 Unnecessary semicolon CssParser.java line 2493 Unnecessary semicolon CssParser.java line 3830 Unnecessary semicolon CssParser.java line 3834 Unnecessary semicolon D3DFrameStats.java line 54 Unnecessary semicolon DefaultCancelButtonTestBase.javaline 304 Unnecessary semicolon DisplacementMap.javaline 108 Unnecessary semicolon DisplacementMap.javaline 243 Unnecessary semicolon DropShadow.java line 148 Unnecessary semicolon DummyTexture.java line 41 Unnecessary semicolon DumpRenderTree.java line 94 Unnecessary semicolon DumpRenderTree.java line 106 Unnecessary semicolon FXCanvas.java line 171 Unnecessary semicolon FXCanvas.java line 1254 Unnecessary semicolon FXDnDInteropN.java line 374 Unnecessary semicolon FireButtonBaseTest.java line 92 Unnecessary semicolon GaussianBlur.java line 83 Unnecessary semicolon Glow.java line 82 Unnecessary semicolon HyperlinkTest.java line 250 Unnecessary semicolon Image.java line 248 Unnecessary semicolon ImageInput.java line 81 Unnecessary semicolon ImageStorage.java line 115 Unnecessary semicolon InnerShadow.javaline 128 Unnecessary semicolon IosImageLoaderFactory.java line 42 Unnecessary semicolon J2DPrinter.java line 865 Unnecessary semicolon J2DPrinterJob.java line 776 Unnecessary semicolon KeyCode.javaline 1177 Unnecessary semicolon KeyCodeMap.java line 62 Unnecessary semicolon KeyFrameTest.java line 131 Unnecessary semicolon KeyFrameTest.java line 136 Unnecessary semicolon KeyFrameTest.java line 141 Unnecessary semicolon KeyFrameTest.java line 146 Unnecessary semicolon KeyFrameTest.java line 218 Unnecessary semicolon KeyFrameTest.java line 223 Unnecessary semicolon KeyFrameTest.java line 228 Unnecessary semicolon LeakTest.java line 187 Unnecessary semicolon LeakTest.java line 348 Unnecessary semicolon LeakTest.java line 474 Unnecessary semicolon LeakTest.java line 205 Unnecessary semicolon LeakTest.java line 210 Unnecessary semicolon LeakTest.java line 215 Unnecessary semicolon LightBaseTest.java line 89 Unnecessary semicolon Lighting.java line 77 Unnecessary semicolon Lighting.java line 164 U
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 19:53:59 GMT, John Hendrikx wrote: >> modules/javafx.base/src/main/java/javafx/beans/property/IntegerPropertyBase.java >> line 56: >> >>> 54: >>> 55: private int value; >>> 56: private ObservableIntegerValue observable = null;; >> >> is there an Eclipse warning for extra semicolon? > > Yes, `potential programming problems / empty statement`. It can sometimes > point to a mistake where a semi colon is used just before a block. Cool. My eclipse finds other places which do look like a bug - see disposed:65 It is in the base - why isn't it this PR? - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 11:53:30 GMT, John Hendrikx wrote: > - Remove unsupported/unnecessary SuppressWarning annotations > - Remove reduntant type specifications (use diamond operator) > - Remove unused or duplicate imports > - Remove unnecessary casts (type is already correct type or can be autoboxed) > - Remove unnecessary semi-colons (at end of class definitions, or just > repeated ones) > - Remove redundant super interfaces (interface that is already inherited) > - Remove unused type parameters > - Remove declared checked exceptions that are never thrown > - Add missing `@Override` annotations @hjohn I changed the title of the bug to make it clearer, please update this PR's title. - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 19:58:31 GMT, John Hendrikx wrote: >> This is in the test shims, so not public API. > > Shims are not public as far as I know. you are right. I just looked at the package "package javafx.event;" and said hmmm... - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 19:32:01 GMT, Kevin Rushforth wrote:
>> modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line
>> 123:
>>
>>> 121:
>>> 122: if (weakReference.get() == null && counter < steps / 3) {
>>> 123: int percentageUsed = (steps - counter) / steps * 100;
>>
>> I suspect this will not produce a desired result.
>> Perhaps it should be
>> (int)(100.0 * (steps - counter) / steps);
>
> Removing the `(int)` cast is a behavior-neutral change. What you are pointing
> out does look a bug, but it is unrelated to this cleanup, so should be filed
> and fixed separately.
Yeah, this looks like a bug, as `(steps - counter) / steps` which are all
`int`s will likely always result in `0`. The cast removal here however does
not alter the calculation, but does point to an error in the author's thought
process. The result is however only used in the log message to inform of a
problem, and so the logic is not otherwise affected.
I can file a separate issue for this, unless we can agree to fix it as part of
this clean-up.
-
PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 19:26:48 GMT, Kevin Rushforth wrote:
>> modules/javafx.base/src/shims/java/javafx/event/EventTypeShim.java line 30:
>>
>>> 28: import java.util.List;
>>> 29:
>>> 30: public class EventTypeShim {
>>
>> this changes public API surface, does it not?
>
> This is in the test shims, so not public API.
Shims are not public as far as I know.
-
PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 17:29:42 GMT, Andy Goryachev wrote:
>> - Remove unsupported/unnecessary SuppressWarning annotations
>> - Remove reduntant type specifications (use diamond operator)
>> - Remove unused or duplicate imports
>> - Remove unnecessary casts (type is already correct type or can be autoboxed)
>> - Remove unnecessary semi-colons (at end of class definitions, or just
>> repeated ones)
>> - Remove redundant super interfaces (interface that is already inherited)
>> - Remove unused type parameters
>> - Remove declared checked exceptions that are never thrown
>> - Add missing `@Override` annotations
>
> modules/javafx.base/src/main/java/javafx/util/converter/DateStringConverter.java
> line 130:
>
>> 128:
>> 129: /** {@inheritDoc} */
>> 130: @SuppressWarnings("removal")
>
> puzzling - why was it marked with @SuppressWarnings("removal") ?
Probably the deprecation for removal warning was first blocked, then later
fixed without removing the suppress warnings. There are currently no calls in
that code that call anything deprecated, so I remove the suppress warnings.
-
PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 17:20:43 GMT, Andy Goryachev wrote: >> - Remove unsupported/unnecessary SuppressWarning annotations >> - Remove reduntant type specifications (use diamond operator) >> - Remove unused or duplicate imports >> - Remove unnecessary casts (type is already correct type or can be autoboxed) >> - Remove unnecessary semi-colons (at end of class definitions, or just >> repeated ones) >> - Remove redundant super interfaces (interface that is already inherited) >> - Remove unused type parameters >> - Remove declared checked exceptions that are never thrown >> - Add missing `@Override` annotations > > modules/javafx.base/src/main/java/javafx/beans/property/IntegerPropertyBase.java > line 56: > >> 54: >> 55: private int value; >> 56: private ObservableIntegerValue observable = null;; > > is there an Eclipse warning for extra semicolon? Yes, `potential programming problems / empty statement`. It can sometimes point to a mistake where a semi colon is used just before a block. - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 19:11:16 GMT, Andy Goryachev wrote:
>> - Remove unsupported/unnecessary SuppressWarning annotations
>> - Remove reduntant type specifications (use diamond operator)
>> - Remove unused or duplicate imports
>> - Remove unnecessary casts (type is already correct type or can be autoboxed)
>> - Remove unnecessary semi-colons (at end of class definitions, or just
>> repeated ones)
>> - Remove redundant super interfaces (interface that is already inherited)
>> - Remove unused type parameters
>> - Remove declared checked exceptions that are never thrown
>> - Add missing `@Override` annotations
>
> modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 123:
>
>> 121:
>> 122: if (weakReference.get() == null && counter < steps / 3) {
>> 123: int percentageUsed = (steps - counter) / steps * 100;
>
> I suspect this will not produce a desired result.
> Perhaps it should be
> (int)(100.0 * (steps - counter) / steps);
Removing the `(int)` cast is a behavior-neutral change. What you are pointing
out does look a bug, but it is unrelated to this cleanup, so should be filed
and fixed separately.
-
PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 17:35:08 GMT, Andy Goryachev wrote:
>> - Remove unsupported/unnecessary SuppressWarning annotations
>> - Remove reduntant type specifications (use diamond operator)
>> - Remove unused or duplicate imports
>> - Remove unnecessary casts (type is already correct type or can be autoboxed)
>> - Remove unnecessary semi-colons (at end of class definitions, or just
>> repeated ones)
>> - Remove redundant super interfaces (interface that is already inherited)
>> - Remove unused type parameters
>> - Remove declared checked exceptions that are never thrown
>> - Add missing `@Override` annotations
>
> modules/javafx.base/src/shims/java/javafx/event/EventTypeShim.java line 30:
>
>> 28: import java.util.List;
>> 29:
>> 30: public class EventTypeShim {
>
> this changes public API surface, does it not?
This is in the test shims, so not public API.
-
PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 11:53:30 GMT, John Hendrikx wrote:
> - Remove unsupported/unnecessary SuppressWarning annotations
> - Remove reduntant type specifications (use diamond operator)
> - Remove unused or duplicate imports
> - Remove unnecessary casts (type is already correct type or can be autoboxed)
> - Remove unnecessary semi-colons (at end of class definitions, or just
> repeated ones)
> - Remove redundant super interfaces (interface that is already inherited)
> - Remove unused type parameters
> - Remove declared checked exceptions that are never thrown
> - Add missing `@Override` annotations
Changes requested by angorya (Author).
modules/javafx.base/src/main/java/javafx/beans/property/IntegerPropertyBase.java
line 56:
> 54:
> 55: private int value;
> 56: private ObservableIntegerValue observable = null;;
is there an Eclipse warning for extra semicolon?
modules/javafx.base/src/main/java/javafx/beans/property/ReadOnlyBooleanProperty.java
line 168:
> 166: }
> 167: };
> 168: };
how did you find these - manually or searching for a "};" pattern?
modules/javafx.base/src/main/java/javafx/util/converter/DateStringConverter.java
line 130:
> 128:
> 129: /** {@inheritDoc} */
> 130: @SuppressWarnings("removal")
puzzling - why was it marked with @SuppressWarnings("removal") ?
modules/javafx.base/src/shims/java/javafx/event/EventTypeShim.java line 30:
> 28: import java.util.List;
> 29:
> 30: public class EventTypeShim {
this changes public API surface, does it not?
modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 123:
> 121:
> 122: if (weakReference.get() == null && counter < steps / 3) {
> 123: int percentageUsed = (steps - counter) / steps * 100;
I suspect this will not produce a desired result.
Perhaps it should be
(int)(100.0 * (steps - counter) / steps);
-
PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 11:53:30 GMT, John Hendrikx wrote: > - Remove unsupported/unnecessary SuppressWarning annotations > - Remove reduntant type specifications (use diamond operator) > - Remove unused or duplicate imports > - Remove unnecessary casts (type is already correct type or can be autoboxed) > - Remove unnecessary semi-colons (at end of class definitions, or just > repeated ones) > - Remove redundant super interfaces (interface that is already inherited) > - Remove unused type parameters > - Remove declared checked exceptions that are never thrown > - Add missing `@Override` annotations This comment might be out of place, but since we started fixing raw type use may be you could take a look at it? I know it's in javafx.graphics, so not technically a part of this PR, but It's pretty weird. If I modify CssParser (any small change would do, even insert a space), Eclipse suddenly gives me a bunch of errors. The constructor ParsedValueImpl[],BackgroundPosition[]>(ParsedValueImpl[], LayeredBackgroundPositionConverter) is undefined CssParser.java line 2929 The constructor ParsedValueImpl[],BackgroundSize[]>(ParsedValueImpl[], LayeredBackgroundSizeConverter) is undefined CssParser.java line 3119 The constructor ParsedValueImpl[],BorderImageSlices[]>(ParsedValueImpl[], SliceSequenceConverter) is undefinedCssParser.java line 3514 The constructor ParsedValueImpl[],BorderStrokeStyle[]>(ParsedValueImpl[], BorderStrokeStyleSequenceConverter) is undefinedCssParser.java line 3172 The constructor ParsedValueImpl[],BorderWidths[]>(ParsedValueImpl[], BorderImageWidthsSequenceConverter) is undefined CssParser.java line 3558 The constructor ParsedValueImpl[],BorderStrokeStyle[]>[],BorderStrokeStyle[][]>(ParsedValueImpl[],BorderStrokeStyle[]>[], LayeredBorderStyleConverter) is undefined CssParser.java line 3187 If I then clean all the projects, the errors disappear. The only solution I found is to convert the offending lines to use raw types. Any ideas? - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 11:53:30 GMT, John Hendrikx wrote: > - Remove unsupported/unnecessary SuppressWarning annotations > - Remove reduntant type specifications (use diamond operator) > - Remove unused or duplicate imports > - Remove unnecessary casts (type is already correct type or can be autoboxed) > - Remove unnecessary semi-colons (at end of class definitions, or just > repeated ones) > - Remove redundant super interfaces (interface that is already inherited) > - Remove unused type parameters > - Remove declared checked exceptions that are never thrown > - Add missing `@Override` annotations One suggestion I'd like to make is to publish the Compiler Errors/Warnings configuration for Eclipse. I have a set of screenshots that produce a 0 err/warn count with the current master. Eclipse also seem to provide a way to export/import these configs, but for some reason export/import stopped working for me around a decade ago. I also noticed two things in general: 1. the default err/warn configuration enables meaningless warnings and disables those that point to the real problems (see [JDK-8289379](https://bugs.openjdk.org/browse/JDK-8289379)) 2. in large code bases with multiple contributors, it makes little sense to enable warnings like usage of raw type or unused imports since they a) don't contribute to code quality and b) are getting re-introduced all the time by people who don't use Eclipse I am giving just two examples, but there are a few other warnings that I think should be disabled. It does not mean we should not attempt to clean up the code base, but at some point the benefit-to-cost ratio is just not that great. - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 11:53:30 GMT, John Hendrikx wrote: > - Remove unsupported/unnecessary SuppressWarning annotations > - Remove reduntant type specifications (use diamond operator) > - Remove unused or duplicate imports > - Remove unnecessary casts (type is already correct type or can be autoboxed) > - Remove unnecessary semi-colons (at end of class definitions, or just > repeated ones) > - Remove redundant super interfaces (interface that is already inherited) > - Remove unused type parameters > - Remove declared checked exceptions that are never thrown > - Add missing `@Override` annotations Marked as reviewed by nlisker (Reviewer). - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 15:19:24 GMT, Kevin Rushforth wrote: > The main thing to be careful of is that we don't touch any public API > signatures, since some of these sort of automated changes can have impact > that we would need to consider. This set of warnings doesn't change method signatures. The "raw types" warning fix will do that, so we will be careful there. - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 11:53:30 GMT, John Hendrikx wrote: > - Remove unsupported/unnecessary SuppressWarning annotations > - Remove reduntant type specifications (use diamond operator) > - Remove unused or duplicate imports > - Remove unnecessary casts (type is already correct type or can be autoboxed) > - Remove unnecessary semi-colons (at end of class definitions, or just > repeated ones) > - Remove redundant super interfaces (interface that is already inherited) > - Remove unused type parameters > - Remove declared checked exceptions that are never thrown > - Add missing `@Override` annotations @andy-goryachev-oracle Please review this as well. - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 11:53:30 GMT, John Hendrikx wrote: > - Remove unsupported/unnecessary SuppressWarning annotations > - Remove reduntant type specifications (use diamond operator) > - Remove unused or duplicate imports > - Remove unnecessary casts (type is already correct type or can be autoboxed) > - Remove unnecessary semi-colons (at end of class definitions, or just > repeated ones) > - Remove redundant super interfaces (interface that is already inherited) > - Remove unused type parameters > - Remove declared checked exceptions that are never thrown > - Add missing `@Override` annotations One other quick comment: since you are removing some suppress warnings for "removal", I'd like to see the results of a build and test with `gradle -PLINT=removal` to ensure that there are no new warnings. - PR: https://git.openjdk.org/jfx/pull/957
Re: RFR: JDK-8297332: Remove easy warnings in base
On Mon, 21 Nov 2022 11:53:30 GMT, John Hendrikx wrote: > - Remove unsupported/unnecessary SuppressWarning annotations > - Remove reduntant type specifications (use diamond operator) > - Remove unused or duplicate imports > - Remove unnecessary casts (type is already correct type or can be autoboxed) > - Remove unnecessary semi-colons (at end of class definitions, or just > repeated ones) > - Remove redundant super interfaces (interface that is already inherited) > - Remove unused type parameters > - Remove declared checked exceptions that are never thrown > - Add missing `@Override` annotations Per our offline discussion, we don't need as careful of a review of these "remove warnings" changes. Since this is the first one, I'd like a second reviewer, mainly to validate the pattern you are using. It should still be a quick review if nothing odd is spotted. The main thing to be careful of is that we don't touch any public API signatures, since some of these sort of automated changes can have impact that we would need to consider. - PR: https://git.openjdk.org/jfx/pull/957
