[GitHub] [tomcat] martin-g opened a new pull request #312: Use StringBuilder instead of StringBuffer

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g opened a new pull request #312: Use StringBuilder instead of StringBuffer

GitBox

martin-g opened a new pull request #312:
URL: https://github.com/apache/tomcat/pull/312


   There is no need of synchronization when it is a method local variable.
   
   Append character instead of String when possible


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] rmaucher commented on pull request #312: Use StringBuilder instead of StringBuffer

GitBox

rmaucher commented on pull request #312:
URL: https://github.com/apache/tomcat/pull/312#issuecomment-653402387


   The "Append character instead of String when possible" seems wrong to me, and you're editing generated code there.


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on pull request #312: Use StringBuilder instead of StringBuffer

GitBox
In reply to this post by GitBox

martin-g commented on pull request #312:
URL: https://github.com/apache/tomcat/pull/312#issuecomment-653404420


   @rmaucher Could you please comment on the changed lines ?
   I am not sure which code is generated and which `append(char)` might be wrong.
   Thanks!


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] pzygielo commented on pull request #312: Use StringBuilder instead of StringBuffer

GitBox
In reply to this post by GitBox

pzygielo commented on pull request #312:
URL: https://github.com/apache/tomcat/pull/312#issuecomment-653434713


   > and which `append(char)` might be wrong.
   
   I suppose, there is change in produced result
   ```
   (1)              retval.append("\\t");
   (2)              retval.append('\t');
   ```
   as in first case "\t" (i.e. two-char string: `\` followed by `t`) is appended, and in second case - tab character ('\t') is.
   Unfortunately I can't tell if there is test to validate what is expected as all smoke test jobs pass.


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on pull request #312: Use StringBuilder instead of StringBuffer

GitBox
In reply to this post by GitBox

martin-g commented on pull request #312:
URL: https://github.com/apache/tomcat/pull/312#issuecomment-653439627


   Thanks, @pzygielo !
   
   Also these seem to be the generated classes!


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on pull request #312: Use StringBuilder instead of StringBuffer

GitBox
In reply to this post by GitBox

martin-g commented on pull request #312:
URL: https://github.com/apache/tomcat/pull/312#issuecomment-653443649


   I've reverted the changes to the generated classes.
   
   What about the DBCP2 classes ? Maybe they should be changed upstream and then copied over here ?


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] markt-asf commented on pull request #312: Use StringBuilder instead of StringBuffer

GitBox
In reply to this post by GitBox

markt-asf commented on pull request #312:
URL: https://github.com/apache/tomcat/pull/312#issuecomment-653453487


   Yes, DBCP2 changes need to be made upstream.


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on pull request #312: Use StringBuilder instead of StringBuffer

GitBox
In reply to this post by GitBox

martin-g commented on pull request #312:
URL: https://github.com/apache/tomcat/pull/312#issuecomment-653508943


   OK. I've reverted the DBCP classes!
   And made some more changes from `.append(String)` to `.append(char)`


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] rmaucher commented on pull request #312: Use StringBuilder instead of StringBuffer

GitBox
In reply to this post by GitBox

rmaucher commented on pull request #312:
URL: https://github.com/apache/tomcat/pull/312#issuecomment-653579373


   Looks good to me now. The change for rewrite is likely the most significant, the rest is probably not doing anything though.


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g merged pull request #312: Use StringBuilder instead of StringBuffer

GitBox
In reply to this post by GitBox

martin-g merged pull request #312:
URL: https://github.com/apache/tomcat/pull/312


   


----------------------------------------------------------------
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:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]