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. >>
