On 08/05/2019 17:30, Christopher Schultz wrote: > Mark, > > On 5/8/19 12:20, Mark Thomas wrote: >> On 08/05/2019 17:16, Christopher Schultz wrote: >>> All, >>> >>> While working on the sort-file-listing enhancement over the >>> weekend, I looked in the Tomcat code to see if we already had a >>> method to split a string by a single character. The closest >>> method I found was org.pache.jasper.compiler.JspUtil which splits >>> on a string, so I rejected it as being a bit too general-purpose >>> for my needs. >>> >>> But it got me looking into the implementation of the method, and >>> it looks a little sketchy. >>> >>> First, it uses a java.util.Vector. Yep, a Vector. Remember >>> those? Synchronized everything? The Vector is not used outside of >>> the method at all -- it's just local and temporary. So, the >>> obvious conclusion is that it should be changed to ArrayList, >>> right? >>> >>> But, modern JVMs are fairly good at (a) processing >>> contention-less locks more quickly and (b) identifying objects >>> which cannot possibly have lock-contention and simply skipping >>> the locking. >>> >>> What's the right move in these cases? Should we change Vector -> >>> ArrayList and "help-out" the compiler and runtime, or should we >>> leave the code alone because "it's fine"? >>> >>> Similar arguments can be made for using StringBuilder over >>> StringBuffer, etc. >>> >>> WDYT? > >> I think the code is cleaner and easier to understand when the code >> matches the intention rather than when the code plus some expected >> compiler optimisation match the intention. So I'd say +1 to making >> these sorts of changes. > > This is kind of my thought as well. > > I also feel like code coming from Apache ought to be an example of the > "right way of doing things" wherever possible. Obviously, that's not a > goal of many projects both within and outside the ASF, but I kind of > feel like we should be able to point to Tomcat code and say "yes, this > is how it should be done". > > If it doesn't look nice, then it should be changed. I'm guessing > that's one of the reasons you went through all the code and changed > try/finally to try-with-resources and Foo<Bar> baz = new Foo<Bar> to > Foo<Bar> baz = new Foo<>() everywhere. I think this is kind of the same.
try-with-resources was partly about reducing code verbosity and partly about ensuring resources were actually closed. Foo<Bar> baz = new Foo<>() was to reduce/eliminate IDE warnings. My preference is to keep the code clean of IDE warnings so when I make a stupid mistake and my IDE tries to point it out it isn't lost in the noise. > Back to JspUtil.split. It's private and used exactly one place. AND > it's only ever called with a single-character String value. So > actually the use-case DID meet my needs for string-splitting for the > sorting. Perhaps I'll re-factor that method a little and then use it > across these two use-cases. Doesn't that mean that DefaultServlet would depend on Jasper? That isn't allowed as the Jasper is meant to be an optional component. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
