Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed double null check with instance operator #5224

Merged
merged 5 commits into from
Jan 12, 2023
Merged

Removed double null check with instance operator #5224

merged 5 commits into from
Jan 12, 2023

Conversation

tbw777
Copy link
Contributor

@tbw777 tbw777 commented Jan 6, 2023

Sometimes found usage of instance operator with variable superflous check for null. But also there is not only variables. Additional null check performs with map.get() or method() and so on.

Also fixed formatting in org.openide.awt.SplittedPanel.java

Sometimes found usage of instance operator with variable superflous  check for null. But also there is not only variables. Additional null check performs with map.get() or method() and so on.
@tbw777
Copy link
Contributor Author

tbw777 commented Jan 6, 2023

Clone of #5205 closed PR with code format fix

Copy link
Member

@sdedic sdedic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A kind request: next time, please split such huge work into more commits or better more pul requests, e.g. one per cluster. More reviewers can work in parallel, and review of a smaller chunk is less error prone.

Looks good except the one forgotten != null not converted.

@@ -120,7 +120,7 @@ public PropertyDialogManager(
if (env != null) {
Object helpID = env.getFeatureDescriptor().getValue(ExPropertyEditor.PROPERTY_HELP_ID);

if ((helpID != null) && helpID instanceof String && (component != null) && component instanceof JComponent) {
if (helpID instanceof String && (component != null) && component instanceof JComponent) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another != null && instanceof :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.
Im entirely agree about task splitting

@sdedic sdedic added the ci:all-tests [ci] enable all tests label Jan 6, 2023
@apache apache locked and limited conversation to collaborators Jan 6, 2023
@apache apache unlocked this conversation Jan 6, 2023
Sometimes found usage of instance operator with variable superflous  check for null. But also there is not only variables. Additional null check performs with map.get() or method() and so on.
@tbw777 tbw777 requested a review from sdedic January 6, 2023 22:35
@tbw777 tbw777 removed their assignment Jan 6, 2023
Sometimes found usage of instance operator with variable superflous  check for null. But also there is not only variables. Additional null check performs with map.get() or method() and so on.
Sometimes found usage of instance operator with variable superflous  check for null. But also there is not only variables. Additional null check performs with map.get() or method() and so on.
Sometimes found usage of instance operator with variable superflous  check for null. But also there is not only variables. Additional null check performs with map.get() or method() and so on.
Copy link
Member

@sdedic sdedic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did just review against the last state 2nd time. No need to further split this one, I see you've already partitioned the next round of code cleanup - thanks !

This is fine by me, but I'd advise that one other reviewer would scan through the changes. As there are so many same-type changes, eyes & brain get tired and may miss something.

@sdedic sdedic assigned sdedic and tbw777 and unassigned sdedic Jan 9, 2023
@tbw777 tbw777 removed their assignment Jan 10, 2023
@tbw777
Copy link
Contributor Author

tbw777 commented Jan 10, 2023

@sdedic I dont see errors. Who can review?

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane to me. Thank you. I restarted the failed CI/CD run "NetBeans / Build Tools on Linux/JDK 8 (pull_request)", which failed in a module, that is known to be flaky.

@matthiasblaesing
Copy link
Contributor

The java.mx.project suite is failing. I ran it locally on JDK 8 and it works - so lets try the next retries (this is number 8).

@matthiasblaesing matthiasblaesing added this to the NB17 milestone Jan 12, 2023
@matthiasblaesing
Copy link
Contributor

Rerunning all tests finally fixed the tests.

@matthiasblaesing matthiasblaesing merged commit 3291c89 into apache:master Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests Code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants