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 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? 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. -chris
signature.asc
Description: OpenPGP digital signature