We used to do linting years ago, and our approach was basically "the past is past, but look for deltas of new bad things added and notify the author". At least for me personally the signal / noise ratio was quite valuable (and not just for my own code; i.e. this wasn't *just* a "Josh Problem" ;) )
I was thinking about this w/the checkstyle additions; we need the ability to bypass these easily for incremental change / test cycles while working on something so devs don't have to pay the "analyze the entire code-base" tax on each small local change. I'm a strong +1 on having these tools in here and on by default for build or jar targets. Maybe it'd be more friendly to have something like "ant unsafe" (or "ant jar-unsafe" so it's adjacent) that just does a quick jar w/out that stuff so it's more discoverable than multiple -D params? On Tue, Nov 8, 2022, at 5:40 AM, Miklosovic, Stefan wrote: > I confused Jacoco to be code analysis tool instead of code coverage one. > Early morning I guess :) I was thinking more about tooling like Sonar. What > do you think about that? Any pros / cons to Spotbugs? > > ________________________________________ > From: Miklosovic, Stefan <stefan.mikloso...@netapp.com> > Sent: Tuesday, November 8, 2022 11:36 > To: dev@cassandra.apache.org > Subject: Re: [DISCUSSION] Add SpotBugs to the build > > Hi David, all > > what is the position of Jacoco in Cassandra as this point? I see it in > build.xml there are some targets and infrastructure around that. > > What is wrong with using Jacoco instead more heavily for static analysis? Why > do we need to introduce this? > > Regards > > ________________________________________ > From: David Capwell <dcapw...@apple.com> > Sent: Tuesday, November 8, 2022 0:10 > To: dev > Subject: [DISCUSSION] Add SpotBugs to the build > > NetApp Security WARNING: This is an external email. Do not click links or > open attachments unless you recognize the sender and know the content is safe. > > > > > I was thinking that it would be good to add SpotBugs > (https://spotbugs.github.io) into our build to help find bugs earlier in the > life cycle. SpotBugs is LGPL but as it is used only in the build and not to > within this project, then this should be fine with Apache. > > The motivation for adding this was from CASSANDRA-17178; the Simulator has > issues with Serializable classes missing serialVersionUID (as we deal with > ClassLoaders; this field is strongly recommend in general for all > Serializable classes), but this project can add more value as there are a > large collection of potential bugs to look out for; below are a few examples > found. > > * Number.valueOf vs Number.parse<size>. In many parts of the code we do > valueOf which returns a boxed value; we then unbox for the usage; this adds > more garbage that isn’t needed > * Using Number.compareTo rather than primitive compare functions (causing > boxing) > * Ignoring return value for functions that don’t have a side effect. This > happens in a few cases where we are building a StringBuilder where we call > .toString but ignore the string… then call it later on > * use of putIfAbsent without looking at the return. This was found in > CacheService where we add the SSTable reader to the cache and assume we win > the race and start using it… rather than using the object that won the race >
