On 26/08/2014 22:26, Christopher Schultz wrote: > Mark, > > On 8/26/14, 5:20 AM, Mark Thomas wrote: >> I have been pinged off-list by Coverity to say that they have set up >> Tomcat with a free account with their static code analysis service. >> >> I think I have the ability to send invitations so if anyone wants to >> take a look at the results, just reply here. >> >> I have taken a quick look and they do appear to have found some valid >> threading issues. There are ~350 issues in total and I don't yet have a >> feel for the false positive rate. > > Wow, this is great. I've used FindBugs before both inside and outside of > ASF projects, but this is ... just amazing.
It certainly appears to be an improvement on what was on offer the last time we were approach by one of the static code analysis firms. Personally, I'm withholding judgement until I get a better feel for what % of the issues raises are ones that are likely to cause problems for our users. > It does catch a lot of sanity-checks and complains about them. I get > DEAD CODE warnings all the time (in FindBugs) for especially JDBC code > when I've caught all possible exceptions and yet still have a finally > block that, for example, checks a Connection reference for null and > closes it in that case. While the code is technically dead, it's > future-proof against someone adding another call that throws a different > type of exception, re-ordering some of the operations, or making some > other change and forgetting to modify the finally block, etc. > > It would be nice to know what the consensus is amongst the team about > what to do in these cases: should all dead code segments be considered > logical oversights and corrected? Or is additional sanity-checking and > future-proofing a good idea? It depends :) I think there are some cases where I'd agree it is a good idea and some where I'd think it was a waste of time and code. > A good example is issue 45040 > (https://scan3.coverity.com:8443/reports.htm#v16818/p10363/fileInstanceId=567725&defectInstanceId=145101&mergedDefectId=45040): > a logical bug in HttpServlet that should probably remain as-is. It's in > the HttpServlet.doOptions method where we build a list of acceptable > HTTP verbs. /Technically/, if ALLOW_GET is set, then ALLOW_HEAD must be > set and therefore checking for "allow" (the string of verbs we're > building) for NULL is illogical. One could argue that ALLOW_HEAD should > be independent of ALLOW_GET -- why can't a servlet implement doHead but > not doGet -- but it probably always makes sense to check for null. > Stated differently: checking for NULL never hurt anybody. I disagree. In this case if the servlet extends HttpServlet and implements doGet(), HttpServlet provides the doHead() implementation. I'd clean that code up and remove the dead code. That said, if have other things higher up my priority list right now. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
