[GitHub] [commons-text] kinow commented on a diff in pull request #430: TEXT-227: removed unnecessary IF statement

["kinow (via GitHub)" <gi...@apache.org>]

kinow commented on code in PR #430:
URL: https://github.com/apache/commons-text/pull/430#discussion_r1264679028


##########
src/main/java/org/apache/commons/text/AlphabetConverter.java:
##########
@@ -87,9 +87,6 @@ public final class AlphabetConverter {
      * @see "http://www.oracle.com/us/technologies/java/supplementary-142654.html"
      */
     private static String codePointToString(final int i) {
-        if (Character.charCount(i) == 1) {

Review Comment:
   Sorry, Gay, I missed the previous message and only saw the notification after Alex's comment above.
   
   I don't recall about `charCount`, but Alex's explanation sounds solid. And +1 to
   
   >Either variation of this method therefore has possible minor advantages. I would be reluctant to change this without a JMH test of performance across the LTS JDK releases for the two variations.
   
   At least so we understand pros & cons of this change.
   
   Also, for performance issues text/imaging/pool, I find it easier to accept changes like this when users have experienced the performance issues or the JMH tests give a clear indication that the change won't bring issues to users with different use cases, different JVM's, different garbage collectors, etc. (i.e. if the change is being recommended by static analyzers and there are pros & cons in JMH test, then I would probably move it to the bottom of the backlog for now.)
   
   Cheers
   
   -Bruno



-- 
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: issues-unsubscribe@commons.apache.org

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