[NETBEANS-4458] - enable use of generics by BradWalker · Pull Request #2194 · apache/netbeans

BradWalker · 2020-06-16T23:32:55Z

A lot of places in the code has generic use commented out.. For example, the following in
LayoutModel.java

 public void unsetSameSize(List/*<String>*/ components, int dimension) {
    Iterator i = components.iterator();
    while (i.hasNext()) {
        String cid = (String)i.next();
        LayoutComponent lc = getLayoutComponent(cid);
        removeComponentFromLinkSizedGroup(lc, dimension);
    }
}

All I've done here is enable it to reduce the warnings and test out the code to make sure it works correctly.

mklaehn

Looks good to me

neilcsmith-net

-1 as of now. This is not a compatible change. Needs proper review / plan, particularly for anything that is public API. @matthiasblaesing made this point already in #1377

platform/openide.nodes/src/org/openide/cookies/InstanceCookie.java

JaroslavTulach · 2020-06-18T09:36:30Z

Many of the changes aren't API changes (like changing local variables and private or package private field/method types). Those can certainly be made, the other ones would deserve some more thoughts. We have just a single try to get the signatures right!

However let me state that it makes no sense to reduce number of warnings without preventing the warnings to ever appear again. E.g. for each module that you want to clean up, turn on "warning as error" mode and then fix (or suppress) all the warnings in the module. Otherwise it is just a useless mangling of the source code. In my 2 Kč opinion.

matthiasblaesing

I have nothing against fixing warnings, but I'd like to keep that separate from changing API. I marked two locations where I saw API, Jaroslav found another, so please check again, that only internal methods or fields are updated.
Friend modules are a bit of a stretch, while not API, I would not say they are fair game to change just because.

ide/xml.core/src/org/netbeans/modules/xml/api/model/ExtendedGrammarQuery.java

ide/xml.text/src/org/netbeans/modules/xml/text/navigator/base/AbstractXMLNavigatorPanel.java

BradWalker · 2020-06-19T03:06:59Z

Many of the changes aren't API changes (like changing local variables and private or package private field/method types). Those can certainly be made, the other ones would deserve some more thoughts. We have just a single try to get the signatures right!

However let me state that it makes no sense to reduce number of warnings without preventing the warnings to ever appear again. E.g. for each module that you want to clean up, turn on "warning as error" mode and then fix (or suppress) all the warnings in the module. Otherwise it is just a useless mangling of the source code. In my 2 Kč opinion.

I'm in agreement with you about this.. But, there are 10s of thousands of warnings.. I'm trying to fix things in a way that only "one thing" gets changed at a time.. That's why I'll do things like just fix the EMPTY_LIST..

I've managed to get our number of warnings down from approx. 26,000 to 19,000.. Once things get more manageable then we can start to turn on "error on warning"..

Hopefully this explains my rational better..

BradWalker · 2020-06-19T03:13:58Z

One more thing.. I do try to stay away from API changes because of the previously mentioned comments. FYI..

matthiasblaesing

Sorry :-) I left two inline comments.

matthiasblaesing · 2020-06-23T20:05:41Z

profiler/lib.profiler/src/org/netbeans/lib/profiler/heap/ClassDumpSegment.java

    }

-    synchronized List /*<JavaClass>*/ createClassCollection() {
+    synchronized List/*<JavaClass>*/ createClassCollection() {


This is package private - I would not consider this API, so could be made generic and then the callsite of this function could be generified.

This is package private - I would not consider this API, so could be made generic and then the callsite of this function could be generified.

Fixed..

matthiasblaesing · 2020-06-23T20:06:50Z

profiler/lib.profiler/src/org/netbeans/lib/profiler/results/BaseCallGraphBuilder.java

    //~ Instance fields ----------------------------------------------------------------------------------------------------------

-    protected List/*<Runnable>*/ afterBatchCommands = new ArrayList<>();
+    protected List<Runnable> afterBatchCommands = new ArrayList<>();


This is a friend API.

This is a friend API.

fixed..

A lot of places in the code has generic use commented out.. For example, the following in LayoutModel.java public void unsetSameSize(List/*<String>*/ components, int dimension) { Iterator i = components.iterator(); while (i.hasNext()) { String cid = (String)i.next(); LayoutComponent lc = getLayoutComponent(cid); removeComponentFromLinkSizedGroup(lc, dimension); } } All I've done here is enable it to reduce the warnings and test out the code to make sure it works correctly.

BradWalker · 2020-12-10T20:13:47Z

-1 as of now. This is not a compatible change. Needs proper review / plan, particularly for anything that is public API. @matthiasblaesing made this point already in #1377

Should be fixed..

jtulach · 2020-12-11T08:18:45Z

enterprise/web.jsf/src/org/netbeans/modules/web/jsf/wizards/TemplateClientPanelVisual.java


    private WizardDescriptor wizardDescriptor;

-    private final Set/*<ChangeListener>*/ listeners = new HashSet(1);


HashSet of size 1 is probably a non-sense. By default it has 75% fill ratio and then it is rehashed.E.g. as soon as first item is added, the set is probably rehashed.

HashSet of size 1 is probably a non-sense.

Just a follow-up comment in case someone comes across this bug report in the future.. I completely agree with your comment! My goal is simply to try and stay away from any "potential" source changes that could be behavioral. Even ones as small as this. Hope that explains why I do what I do..

jtulach · 2020-12-11T08:19:32Z

The current state is integratable, as far as I can say.

matthiasblaesing

Looks sane - lets get this in :-)

BradWalker · 2020-12-14T16:39:34Z

Hey @junichi11 , how can I add the "code cleanup" label? I don't seem to have permission..

junichi11 · 2020-12-14T16:50:02Z

@BradWalker Well, only committer... So, if you would like to add a label, please ask someone (e.g. reviewer). I add it when I notice it is missing.

mklaehn approved these changes Jun 18, 2020

View reviewed changes

neilcsmith-net requested review from JaroslavTulach, matthiasblaesing and neilcsmith-net June 18, 2020 08:50

neilcsmith-net requested changes Jun 18, 2020

View reviewed changes

JaroslavTulach reviewed Jun 18, 2020

View reviewed changes

platform/openide.nodes/src/org/openide/cookies/InstanceCookie.java Outdated Show resolved Hide resolved

matthiasblaesing requested changes Jun 18, 2020

View reviewed changes

ide/xml.core/src/org/netbeans/modules/xml/api/model/ExtendedGrammarQuery.java Outdated Show resolved Hide resolved

ide/xml.text/src/org/netbeans/modules/xml/text/navigator/base/AbstractXMLNavigatorPanel.java Outdated Show resolved Hide resolved

BradWalker force-pushed the enable_generics branch from d8d5715 to 0b61d12 Compare June 19, 2020 03:10

BradWalker requested review from matthiasblaesing and neilcsmith-net June 19, 2020 03:12

matthiasblaesing reviewed Jun 23, 2020

View reviewed changes

BradWalker force-pushed the enable_generics branch from 0b61d12 to 534be04 Compare December 10, 2020 20:10

BradWalker requested a review from matthiasblaesing December 11, 2020 02:05

jtulach reviewed Dec 11, 2020

View reviewed changes

matthiasblaesing approved these changes Dec 11, 2020

View reviewed changes

matthiasblaesing merged commit 38172f4 into apache:master Dec 11, 2020

junichi11 added this to the 12.3 milestone Dec 12, 2020

BradWalker deleted the enable_generics branch December 14, 2020 16:14

junichi11 added the Code cleanup Label for cleanup done on the Netbeans IDE label Dec 14, 2020


		private WizardDescriptor wizardDescriptor;

		private final Set/<ChangeListener>/ listeners = new HashSet(1);

Conversation

BradWalker commented Jun 16, 2020

Uh oh!

mklaehn left a comment

Choose a reason for hiding this comment

Uh oh!

neilcsmith-net left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JaroslavTulach commented Jun 18, 2020

Uh oh!

matthiasblaesing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BradWalker commented Jun 19, 2020

Uh oh!

BradWalker commented Jun 19, 2020

Uh oh!

matthiasblaesing left a comment

Choose a reason for hiding this comment

Uh oh!

matthiasblaesing Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

BradWalker Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

matthiasblaesing Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

BradWalker Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

BradWalker commented Dec 10, 2020

Uh oh!

jtulach Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

BradWalker Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

jtulach commented Dec 11, 2020

Uh oh!

matthiasblaesing left a comment

Choose a reason for hiding this comment

Uh oh!

BradWalker commented Dec 14, 2020

Uh oh!

junichi11 commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

junichi11 commented Dec 14, 2020 •

edited

Loading