You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/07/10 14:36:49 UTC
[GitHub] [tomcat] martin-g opened a new pull request #321: Simplify some #toString() methods
martin-g opened a new pull request #321:
URL: https://github.com/apache/tomcat/pull/321
Do not use StringBuilder when String concatenation is just fine.
Append char instead of String when possible.
Fix the toString of ExampleFilter
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on pull request #321: Simplify some #toString() methods
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on pull request #321:
URL: https://github.com/apache/tomcat/pull/321#issuecomment-656880783
> Bad idea on adding concatenation (rest is good).
I don't see any "added" concatenation. Some uses of `StringBuilder` have been replaced with string concatenation which is a win for several reasons:
1. It's much easier to read + debug
2. The compiler can convert to using StringBuffer (old) StringBuilder (newer) or StringConcatFactory (newest) instead of using the latest technique some programmer most-recently read about
> That causes lots of extra object creation needlessly.
Citation needed. The compiler converts this:
return "foo" + 25 + "bar";
Into roughly this:
StringBuilder sb = new StringBuilder("foo");
sb.append(23);
sb.append("bar");
return sb.toString();
So going backward doesn't add anything.
> Concatenation is long known to be a problem.
A problem which the compiler is perfectly capable of handling. This part is about readability and maintainability, which are being improved by the patch IMO.
> This PR is doing 2 things, fixing real issues and then adding unnecessary concatenation in others.
Please be specific: point to a single place where concatenation is being added where it did not previously exist.
> There are better ways to do that without string builder
Such as?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on pull request #321: Simplify some #toString() methods
Posted by GitBox <gi...@apache.org>.
rmaucher commented on pull request #321:
URL: https://github.com/apache/tomcat/pull/321#issuecomment-656888828
Optimizing isn't important here so this PR is an improvement even if it was slower.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] martin-g merged pull request #321: Simplify some #toString() methods
Posted by GitBox <gi...@apache.org>.
martin-g merged pull request #321:
URL: https://github.com/apache/tomcat/pull/321
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] hazendaz commented on pull request #321: Simplify some #toString() methods
Posted by GitBox <gi...@apache.org>.
hazendaz commented on pull request #321:
URL: https://github.com/apache/tomcat/pull/321#issuecomment-656784397
Bad idea on adding concatenation (rest is good). That causes lots of extra object creation needlessly. Concatenation is long known to be a problem. This PR is doing 2 things, fixing real issues and then adding unnecessary concatenation in others. While I'm not part of this team, I would suggest this be changed to only address the real issues and not add in concatenation. There are better ways to do that without string builder but this is likely this way to allow easy backports still to 7.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on pull request #321: Simplify some #toString() methods
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on pull request #321:
URL: https://github.com/apache/tomcat/pull/321#issuecomment-657831187
> @ChristopherSchultz [...] Others switched it to concatenation.
I don't see a single case of concatenation where it wasn't already there (in some form or other). Be specific if you think there is a problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] martin-g commented on pull request #321: Simplify some #toString() methods
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #321:
URL: https://github.com/apache/tomcat/pull/321#issuecomment-657513641
@hazendaz Thanks for your comments! See https://www.guardsquare.com/en/blog/string-concatenation-java-9-untangling-invokedynamic - Java 8 already uses StringBuilder. Java 9+ makes it more flexible but still preserves the speed of concatenating strings. But as pointed by others those are not hot paths in Tomcat. So this change is mostly for better readability
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] hazendaz commented on pull request #321: Simplify some #toString() methods
Posted by GitBox <gi...@apache.org>.
hazendaz commented on pull request #321:
URL: https://github.com/apache/tomcat/pull/321#issuecomment-656901567
@ChristopherSchultz Sorry my point was that there was more than just one thing going on in this change. Some of the stringBuilders didn't do anything other than add entirely static items so those are all good cleanups on their own as no concatenation nor string builder was necessary. That was what I was referring to. Others switched it to concatenation. The compiler does handle it if the items are entirely static. However, if they are not, it's left to the runtime to figure out. If optimization doesn't matter in this area that is fine. Just pointing that out. As to better way to concat, use Strings class to join but that is java 8 and I'm sure its always better to keep tomcat so it's easily backported to prior versions that are not on java 8.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org