-
Notifications
You must be signed in to change notification settings - Fork 846
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
Code Cleanup/Deprecation fixes: Class#newInstance, underscore identifier, better temp files, avoid toUpper/LowerCase and more #7053
Conversation
e134bc6
to
cc1f74b
Compare
cc1f74b
to
f9a2717
Compare
@junichi11 @troizet want to look through the PHP commit? (or any other commits you are interested in) |
php/php.dbgp/src/org/netbeans/modules/php/dbgp/models/ThreadsModel.java
Outdated
Show resolved
Hide resolved
We have the PHP feature branch for NB22. So, please merge this after we merge the PHP feature branch. (I'm going to merge it into the master after NB21 is released.) Thanks. cc: @tmysik |
@junichi11 yes saw it already, I won't merge before some of the larger pending PRs are integrated. |
Thanks a lot for doing this, Michael! |
I think a good way of reviewing cleanups like this is to let github generate a diff patch for a commit, e.g: Also for the reviewers: please mention what parts you reviewed in the approve comment so that I get an idea of what is reviewed and what is not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I eyeballed samples from all commits. The changes looks sane to me and nothing out of the ordinary jumped out. Thank you.
} | ||
}); | ||
projectsArray.sort((Object o1, Object o2) -> | ||
getDisplayName((Provider) o1).toLowerCase().compareTo(getDisplayName((Provider) o2).toLowerCase()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't use compareToIgnoreCase
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh interesting, I didn't know that this method existed. I did only transform to equalsIgnoreCase()
I might do this at a later point since this PR is already large enough.
But i am going to add this to my hints collection for sure https://github.com/mbien/jackpot-inspections
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking closer at the method it likely has unintended side effects since it sorts the array in-place, and then it returns a copy via toArray()
(which makes no sense).
other implementations do copy first, sort it and return the copy:
https://github.com/search?q=repo%3Aapache%2Fnetbeans%20getSortedProjects&type=code
should I change it to:
public static Provider[] getSortedProjects(Provider[] projects) {
Provider[] sorted = Arrays.copyOf(projects, projects.length);
Arrays.sort(sorted, (p1, p2) -> getDisplayName(p1).compareToIgnoreCase(getDisplayName(p2)));
return sorted;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added it as extra commit (the other ProjectUtilities
class in ...profiler.nbimpl.project is deprecated and not used for the getSortedProject
utility method, only the new ProjectUtilities
had this behavior)
profiler/profiler.ppoints/src/org/netbeans/modules/profiler/ppoints/LoadGenProfilingPoint.java
Outdated
Show resolved
Hide resolved
java/debugger.jpda/src/org/netbeans/modules/debugger/jpda/models/AbstractObjectVariable.java
Outdated
Show resolved
Hide resolved
java/debugger.jpda/src/org/netbeans/modules/debugger/jpda/models/AbstractObjectVariable.java
Outdated
Show resolved
Hide resolved
java/form/src/org/netbeans/modules/form/palette/PaletteUtils.java
Outdated
Show resolved
Hide resolved
java/java.api.common/src/org/netbeans/modules/java/api/common/project/ui/LibrariesNode.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I've glanced through each commit. Thank you for your work!
f9a2717
to
5f7ce19
Compare
Files.createTempFile uses more restrictive file permissions if possible. project wide refactoring, skipped most tests and some dead code to shrink the change set.
String.toUpper/LowerCase() without setting Locale.ROOT can lead to unintended results on certain locales. String.equalsIgnoreCase() can be sometimes used instead and the method does not depend on the locale. This reduces the toUpper/LowerCase usage a bit further before the great Locale.ROOT refactoring starts.
- '_' is a reserved keyword since java 8 and causes javac warnings
- c.getDeclaredConstructor().newInstance() can be used instead. - simplified exception handling in related code
- more fluent/direct - one less import in many cases
- zeroing step is skipped with col.toArray(new T[0]) or (T::new) - easier to optimize for the JVM - shorter/easier to read
- zeroing step is skipped with col.toArray(new T[0]) or (T::new) - easier to optimize for the JVM - shorter/easier to read
- zeroing step is skipped with col.toArray(new T[0]) or (T::new) - easier to optimize for the JVM - shorter/easier to read
- zeroing step is skipped with col.toArray(new T[0]) or (T::new) - easier to optimize for the JVM - shorter/easier to read
- zeroing step is skipped with col.toArray(new T[0]) or (T::new) - easier to optimize for the JVM - shorter/easier to read
- zeroing step is skipped with col.toArray(new T[0]) or (T::new) - easier to optimize for the JVM - shorter/easier to read
- original impl sorted the view of the input array in place and made a copy afterwards, which is unusual and likely unintended - new impl copies input, sorts it and returns the copy
5f7ce19
to
0a3a85c
Compare
planning to merge once green again the other PRs should do fine hopefully, I don't expect many conflicts there:
thanks for review! |
Contains project wide cleanups/refactorings/deprecation fixes:
Files#createTempFile
overFile#createTempFile
.Files.createTempFile
uses more restrictive file permissions if possible.String.equalsIgnoreCase()
overtoUpper/LowerCase
patternsString.toUpper/LowerCase()
without settingLocale.ROOT
can lead to unintended results on certain locales._
is a reserved keyword since java 8Class#newInstance
deprecation warningsc.getDeclaredConstructor().newInstance()
can be used insteadlist.sort(comp)
overCollections.sort(list, comp)
Comparator
, we could in theory mapCollections.sort(list)
tolist.sort(null)
, but I left those cases out since this might look confusing)col.toArray()
col.toArray(new T[0])
or(T::new)
All changes were made using either jackpot scripts or built-in NetBeans code transformations. The
Class#newInstance
change needed some adjustments per hand, since due to slightly different exception handling.I looked through the patch files twice, except the
toArray()
change which I only looked through once due to its size. I only had to tweak minor formatting anomalies here and there.