You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2022/03/07 18:22:36 UTC

[GitHub] [netbeans] mbien opened a new pull request #3724: project wide refactoring: avoid String.replaceAll() if possible.

mbien opened a new pull request #3724:
URL: https://github.com/apache/netbeans/pull/3724


   WIP project wide optimization (more low hanging fruits for micro optimizations)
   
   `String.replaceAll()` is expecting regexps as arguments, while `String.replace()` can be used for simple substitutions. Due to its sub-optimal name `replaceAll` is sometimes unintentionally used even without any regexps involved.
   
   simple `replace()`
    - is cleaner / doesn't require additional character escaping
    - and safer / less error prone
    - is 10-20x faster +  produces less garbage
    
   this might fix some bugs too, for example `localizedBundlepath.replaceAll(".properties", ...)` didn't escape the '.' which could potentially cause issues with certain filenames.
   
   used [jackpot rules](https://github.com/mbien/jackpot-inspections/blob/0832be385a6512eceebeef7651423d71c5315b90/ProbableBugs.hint#L25-L30) for the simple cases and to find candidates, adjustments were made manually.
   
   looked through it at least twice by now + ran some manual tests with a dev build.
   
   planning to squash the commits after review - lets see what the tests say


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists

[GitHub] [netbeans] mbien commented on a change in pull request #3724: project wide refactoring: avoid String.replaceAll() if possible.

Posted by GitBox <gi...@apache.org>.
mbien commented on a change in pull request #3724:
URL: https://github.com/apache/netbeans/pull/3724#discussion_r822578716



##########
File path: apisupport/apisupport.project/src/org/netbeans/modules/apisupport/project/spi/BrandingModel.java
##########
@@ -719,7 +719,7 @@ private String backslashesToSlashes (String text) {
         for (BundleKey bundleKey : internationalizedResourceBundleKeys) {
             String localizedBundlepath = bundlepath;
             if(!localizedBundlepath.endsWith("_" + this.locale.toString() + ".properties"))
-                localizedBundlepath = bundlepath.replaceAll(".properties", "_" + this.locale.toString() + ".properties");

Review comment:
       dot in `".properties"` should have been escaped: `"\.properties"` (multiple occurrences in this class)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists

[GitHub] [netbeans] mbien commented on pull request #3724: project wide refactoring: avoid String.replaceAll() if possible.

Posted by GitBox <gi...@apache.org>.
mbien commented on pull request #3724:
URL: https://github.com/apache/netbeans/pull/3724#issuecomment-1063513808


   > Looks good to me.
   
   thanks for reviewing!
   > 
   > One thing to note: `replace(String, String)` in case of both Strings being of length one could instead use `replace(char, char)`.
   
   This used to be a good advice, but this method is intensified and has the same performance as the char variant on modern JVMs. The only String/char based method left which still behaves differently performance wise is `indexOf`. So its more of a matter of preference now - no longer a performance optimization.
   
   sidenote: I actually tried to add a fast path for `indexOf` https://github.com/openjdk/jdk/pull/6509 but it turned out that under certain circumstances (String lengths) the String variant is actually faster than the char variant due to different vectorization heuristics (this shouldn't happen). This would have to be fixed first otherwise it is not possible to make assumptions of which path is actually faster.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists

[GitHub] [netbeans] mbien merged pull request #3724: project wide refactoring: avoid String.replaceAll() if possible.

Posted by GitBox <gi...@apache.org>.
mbien merged pull request #3724:
URL: https://github.com/apache/netbeans/pull/3724


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists

[GitHub] [netbeans] mbien commented on a change in pull request #3724: project wide refactoring: avoid String.replaceAll() if possible.

Posted by GitBox <gi...@apache.org>.
mbien commented on a change in pull request #3724:
URL: https://github.com/apache/netbeans/pull/3724#discussion_r822578716



##########
File path: apisupport/apisupport.project/src/org/netbeans/modules/apisupport/project/spi/BrandingModel.java
##########
@@ -719,7 +719,7 @@ private String backslashesToSlashes (String text) {
         for (BundleKey bundleKey : internationalizedResourceBundleKeys) {
             String localizedBundlepath = bundlepath;
             if(!localizedBundlepath.endsWith("_" + this.locale.toString() + ".properties"))
-                localizedBundlepath = bundlepath.replaceAll(".properties", "_" + this.locale.toString() + ".properties");

Review comment:
       dot in ".properties" should have been escaped: "\.properties" (multiple occurrences in this class)

##########
File path: java/performance/src/org/netbeans/modules/performance/guitracker/ActionTracker.java
##########
@@ -758,11 +758,11 @@ public void exportAsXML(Document style, PrintStream out)
     }
 
     private static String getShortenName(String name) {
-        name = name.replaceAll("javax.swing", "j");
-        name = name.replaceAll("org.netbeans.modules", "o.n.m");
-        name = name.replaceAll("org.netbeans", "o.n");
-        name = name.replaceAll("org.openide.awt", "o.o.a");
-        name = name.replaceAll("org.openide", "o.o");

Review comment:
       same here. "." not escaped

##########
File path: ide/editor.search/src/org/netbeans/modules/editor/search/SearchComboBoxEditor.java
##########
@@ -176,14 +176,14 @@ public static void changeToOneLineEditorPane(JEditorPane editorPane) {
                     @Override
                     public void insertString(DocumentFilter.FilterBypass fb, int offset, String string, AttributeSet attr) throws BadLocationException {
                         if (string != null) {
-                            fb.insertString(offset, string.replaceAll("\\t", "").replaceAll("\\n", ""), attr); //NOI18N

Review comment:
       i don't think you have to escape `\n` or `\t` but it doesn't seem to hurt.
   ```java
           System.out.println("a\nb\nc".replaceAll("\\n", ""));
           System.out.println("a\nb\nc".replaceAll("\n", ""));
           System.out.println("a\nb\nc".replace("\n", ""));
   ```
   give equal results




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists

[GitHub] [netbeans] mbien commented on a change in pull request #3724: project wide refactoring: avoid String.replaceAll() if possible.

Posted by GitBox <gi...@apache.org>.
mbien commented on a change in pull request #3724:
URL: https://github.com/apache/netbeans/pull/3724#discussion_r822578716



##########
File path: apisupport/apisupport.project/src/org/netbeans/modules/apisupport/project/spi/BrandingModel.java
##########
@@ -719,7 +719,7 @@ private String backslashesToSlashes (String text) {
         for (BundleKey bundleKey : internationalizedResourceBundleKeys) {
             String localizedBundlepath = bundlepath;
             if(!localizedBundlepath.endsWith("_" + this.locale.toString() + ".properties"))
-                localizedBundlepath = bundlepath.replaceAll(".properties", "_" + this.locale.toString() + ".properties");

Review comment:
       dot in ".properties" should have been escaped (multiple occurrences in this class)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists

[GitHub] [netbeans] mbien commented on pull request #3724: project wide refactoring: avoid String.replaceAll() if possible.

Posted by GitBox <gi...@apache.org>.
mbien commented on pull request #3724:
URL: https://github.com/apache/netbeans/pull/3724#issuecomment-1063527738


   squashed everything to one commit. going to merge tomorrow if everything is still green.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists