This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Ignore notes along with a warning when processing NOLINT
ClosedPublic

Authored by nkakuev on Nov 1 2016, 3:39 PM.

Details

Summary

When clang-tidy suppresses a warning, ignored by NOLINT, it doesn't suppress related notes. This leads to an assertion/crash, as described in this bug: https://llvm.org/bugs/show_bug.cgi?id=30565

The minimal reproducible example is as follows (run with 'clang-tidy source.cpp -- -c'):

  • header.h:
void TriggerWarning(const int& In, int& Out) {
  if (In > 123) // NOLINT
    Out = 123;
}
  • source.cpp:
#include "header.h"
void MaybeInitialize(int &Out) {
  int In;
  TriggerWarning(In, Out);
}

Suppressing notes after suppressing a warning fixes the problem and produces a consistent diagnostics output.

Diff Detail

Event Timeline

nkakuev updated this revision to Diff 76639.Nov 1 2016, 3:39 PM
nkakuev retitled this revision from to [clang-tidy] Ignore notes along with a warning when processing NOLINT.
nkakuev updated this object.
nkakuev added reviewers: alexfh, mgehre.
nkakuev updated this object.
nkakuev added a subscriber: cfe-commits.
malcolm.parsons added inline comments.
clang-tidy/ClangTidyDiagnosticConsumer.cpp
312

Change ShouldIgnoreNotes to a class member and rename to LastErrorWasIgnored or similar.

nkakuev updated this revision to Diff 76718.Nov 2 2016, 9:14 AM
nkakuev added a reviewer: malcolm.parsons.

Fixed review comments: replaced a static variable with a class member.

malcolm.parsons accepted this revision.Nov 2 2016, 10:34 AM
malcolm.parsons edited edge metadata.

LGTM, with typo below fixed.

test/clang-tidy/nolint.cpp
36

Is header.h supposed to be trigger_warning.h?

This revision is now accepted and ready to land.Nov 2 2016, 10:34 AM
nkakuev updated this revision to Diff 76769.Nov 2 2016, 12:37 PM
nkakuev edited edge metadata.
nkakuev marked an inline comment as done.

Fix a typo.

@malcolm.parsons, can you please commit this patch? I don't have commit rights.

Committed as r285861.
Thanks!

alexfh added inline comments.Nov 7 2016, 4:40 PM
test/clang-tidy/nolint.cpp
36

The test is too brittle: if the functionality breaks after the text of the message changes, it will still pass.

Two ways to deal with it:

  1. add a positive test to ensure this pattern actually triggers a warning
  2. make the CHECK-NOT pattern broader: // CHECK-NOT: warning:.

@alexfh I'll fix it in a separate review.

nkakuev marked an inline comment as done.
nkakuev added inline comments.
test/clang-tidy/nolint.cpp
36

Fixed in review D26466.