On Kudu we've hooked it up as a precommit, but without any "vote". That is to say, it will add a gerrit comment with any warnings/diagnostics, but doesn't add a '-1' which prevents merge. This has been useful for pointing out style issues or missed optimizations, but we've been able to adopt it gradually without a big-bang "tidy everything" kind of commit which can be disruptive to in-flight patches.
Typically we expect patch contributors to address the review comments from clang-tidy just like they'd address review comments from a human reviewer (which might include ignoring the comment if it's just some code that was moved from one place to another rather than new code) Here's an example of the type of comments it leaves: https://gerrit.cloudera.org/#/c/5252/1/src/kudu/master/catalog_manager.cc@3296 -Todd On Tue, Nov 29, 2016 at 10:06 AM, Jim Apple <[email protected]> wrote: > Also, individual warning can be suppressed with "// NOLINT" (or with > "#pragma clang diagnostic ignored" for tidy checks that are also > compiler warnings) > > On Tue, Nov 29, 2016 at 10:01 AM, Sailesh Mukil <[email protected]> > wrote: > > On Tue, Nov 29, 2016 at 9:50 AM, Henry Robinson <[email protected]> > wrote: > > > >> On 29 November 2016 at 08:06, Jim Apple <[email protected]> wrote: > >> > >> > Should we add to our pre-merge testing (aka GVM, aka GVO) some tests > >> > that don't run impalad, but only build it or check for correctness? > >> > > >> > For instance: > >> > > >> > 1. bin/run_clang_tidy.sh - should we force our code to always be > >> > clang-tidy? > >> > > >> > >> I don't have enough experience of the tool to know if this likely to be > a > >> help or hindrance. > >> > >> > >> > > +1 for this. My opinion is unless we foresee some patches that would fail > > clang-tidy but still be considered a valid patch by us, we should have > this > > as a pre-commit test. > > > >> > >> > 2. bin/check-rat-report.py - uses Apache RAT to check that our code > >> > has proper license headers > >> > > >> > >> +1 > >> > >> > >> > > >> > 3. Other buildall.sh options - in the past we have broken -asan, > >> > -release, or -so without breaking the pre-commit test. > >> > > >> > >> If all can be tested for 'free' wrt to wall-clock-time, then sure. But > if > >> that's not possible, I'd only consider building -release, and maybe not > >> even that. -asan failures are infrequent enough that I don't expect it > to > >> be worth the extra time it would add to the pre-commit build. > >> > >> -so is less important to me. > >> > >> > >> > > >> > 4. Docs build > >> > > >> > I think I can do these without increasing the end-to-end time it takes > >> > to run the tests, by doing them in parallel. One issue I see is that > >> > committers who see their change as minor and merge it manually, > >> > without pre-merge testing, might break clang-tidy or RAT tests. > >> > > >> > >> For that reason, perhaps a separate docs build makes the most sense. > >> > -- Todd Lipcon Software Engineer, Cloudera
