clang-tidy: what might cause NOLINT comments to not be respected?

Viewed 1165

I've created a PR for the opensource Traffic Server project. They run clang-tidy as a part of their CI. My change exposes a new file to clang-tidy which is now being flagged with a use-after-move warning. Here's the test code, pinned at the commit that my change is based upon:

https://github.com/apache/trafficserver/blob/415cd8f/tests/tools/plugins/test_cppapi.cc#L115

Here's a copy of that code, with a comment added showing where it complains:

void
f()
{
  TestCont::Mutex m(TSMutexCreate());

  TestCont c(m);

  ALWAYS_ASSERT(!!c)
  ALWAYS_ASSERT(c.asTSCont() != nullptr)
  ALWAYS_ASSERT(c.mutex() == m)

  TestCont c2(std::move(c));

  ALWAYS_ASSERT(!!c2)
  ALWAYS_ASSERT(c2.asTSCont() != nullptr)   // <--- Complains here
  ALWAYS_ASSERT(c2.mutex() == m)

  ALWAYS_ASSERT(!c)
  ALWAYS_ASSERT(c.asTSCont() == nullptr)
  ALWAYS_ASSERT(c.mutex() == nullptr)

So the complaint makes sense, c2 is used after a move. But in this case, TestCont explicitly supports use after a move by design and the test is intentionally exercising this to make sure its state is as expected.

Thus, this is a situation for which NOLINT and NOLINTNEXTLINE are created. So I applied such comments like this (clearly I've added more comments than I should need to out of desperation):

  // NOLINTNEXTLINE
   ALWAYS_ASSERT(!c) // NOLINT
   // We turn off the clang-tidy warning about this being a use after move
   // because that is the intention of the test. Continuations support use after
   // move.
   // NOLINTNEXTLINE
   const auto cont_after_move = c.asTSCont(); // NOLINT
   ALWAYS_ASSERT(cont_after_move == nullptr)
   // We turn off the clang-tidy warning about this being a use after move
   // because that is the intention of the test. Continuations support use after
   // move.
   // NOLINTNEXTLINE
   const auto mutex_after_move = c.mutex(); // NOLINT
   ALWAYS_ASSERT(mutex_after_move == nullptr)

Notice that I separated c.asTSCont() as a separate call in case things were being confused by the ALWAYS_ASSERT macro. Yet clang-tidy still complains. Here's the latest jenkins run output:

https://ci.trafficserver.apache.org/job/clang-analyzer-github/12245/console

tools/plugins/test_cppapi.cc:124:32: warning: Method called on moved-from object 'c'
  const auto cont_after_move = c.asTSCont(); // NOLINT
                               ^~~~~~~~~~~~
tools/plugins/test_cppapi.cc:151:33: warning: Method called on moved-from object 'c2'
  const auto cont2_after_move = c2.asTSCont(); // NOLINT
                                ^~~~~~~~~~~~~
2 warnings generated.

Right there in the warning output is the NOLINT comment. What am I doing wrong? Why isn't clang-tidy respsecting NOLINT?

The clang-tidy version is 10.0.0. Here's the clang-analyzer report in case it's helpful:

https://ci.trafficserver.apache.org/clang-analyzer/github/6945/2020-06-25-081635-12865-1/report-33708c.html#EndPath

Here's a copy and paste of the clang-tidy invocation:

clang -cc1 -triple x86_64-unknown-linux-gnu -analyze -disable-free -disable-llvm-verifier -discard-value-names -main-file-name test_cppapi.cc -analyzer-store=region -analyzer-opt-analyze-nested-blocks -analyzer-checker=core -analyzer-checker=apiModeling -analyzer-checker=unix -analyzer-checker=deadcode -analyzer-checker=cplusplus -analyzer-checker=security.insecureAPI.UncheckedReturn -analyzer-checker=security.insecureAPI.getpw -analyzer-checker=security.insecureAPI.gets -analyzer-checker=security.insecureAPI.mktemp -analyzer-checker=security.insecureAPI.mkstemp -analyzer-checker=security.insecureAPI.vfork -analyzer-checker=nullability.NullPassedToNonnull -analyzer-checker=nullability.NullReturnedFromNonnull -analyzer-output plist -w -setup-static-analyzer -mrelocation-model pic -pic-level 2 -mthread-model posix -mframe-pointer=none -relaxed-aliasing -fmath-errno -fno-rounding-math -masm-verbose -mconstructor-aliases -munwind-tables -target-cpu x86-64 -target-feature +cx16 -dwarf-column-info -fno-split-dwarf-inlining -debugger-tuning=gdb -resource-dir /opt/llvm/lib64/clang/10.0.0 -D HAVE_CONFIG_H -I . -I ../include -D linux -D _LARGEFILE64_SOURCE=1 -D _COMPILE64BIT_SOURCE=1 -D _REENTRANT -D __STDC_LIMIT_MACROS=1 -D __STDC_FORMAT_MACROS=1 -I /var/jenkins/workspace/clang-analyzer-github/src/proxy/api -I /var/jenkins/workspace/clang-analyzer-github/src/proxy/api -I /var/jenkins/workspace/clang-analyzer-github/src/include/cppapi/include -I /var/jenkins/workspace/clang-analyzer-github/src/lib/cppapi/include -I /var/jenkins/workspace/clang-analyzer-github/src/include -I /var/jenkins/workspace/clang-analyzer-github/src/lib -D _GNU_SOURCE -D OPENSSL_NO_SSL_INTERN -I /opt/llvm/include/c++/v1 -D PIC -internal-isystem /opt/llvm/bin/../include/c++/v1 -internal-isystem /usr/local/include -internal-isystem /opt/llvm/lib64/clang/10.0.0/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -O3 -Wno-deprecated-declarations -Wno-ignored-qualifiers -Wno-unused-parameter -Wno-invalid-offsetof -std=c++17 -fdeprecated-macro -fdebug-compilation-dir /var/jenkins/workspace/clang-analyzer-github/src/tests -ferror-limit 19 -fmessage-length 0 -fgnuc-version=4.2.1 -fobjc-runtime=gcc -fcxx-exceptions -fexceptions -fdiagnostics-show-option -vectorize-loops -vectorize-slp -analyzer-checker alpha.unix.cstring.BufferOverlap -analyzer-checker alpha.core.BoolAssignment -analyzer-checker alpha.core.CastSize -analyzer-checker alpha.core.SizeofPtr -analyzer-output=html -faddrsig -o /CA/clang-analyzer/github/6945/2020-06-25-081635-12865-1 -x c++ tools/plugins/test_cppapi.cc
1 Answers

In the end I was confused between clang-tidy and clang-analyzer. NOLINT addresses clang-tidy issues, but I had to suppress clang-analyzer. I did so using the suggestion here:

https://clang-analyzer.llvm.org/faq.html#exclude_code

The following directives quieted the warnings for me:

#ifndef __clang_analyzer__
// Code not to be analyzed
#endif
Related