[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 22 06:38:40 PST 2017


aaron.ballman added inline comments.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:21-22
 #include "ClangTidyOptions.h"
+#include "ClangTidy.h"
+#include "ClangTidyModule.h"
 #include "clang/AST/ASTDiagnostic.h"
----------------
Please keep the includes in sorted order.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:132
+        ++BracketIndex;
+        const size_t BracketEndIndex = Line.find(')', BracketIndex);
+        if ((ClosingBracketFound = BracketEndIndex != StringRef::npos)) {
----------------
We don't typically mark local, non-pointer types as `const`.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:161
+
+  const SmallVector<StringRef, 1>& getCheckNames() const {
+    assert(isNolintFound());
----------------
The `&` should bind to the right. However, instead of returning the concrete container, why not expose a range interface instead?


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:261
+  struct NolintTargetInfoHasher {
+    size_t operator() (const NolintTargetInfo &i) const {
+      return llvm::hash_combine(i.LineNumber, i.CheckName);
----------------
Spurious space after `operator()`. `i` does not match our coding conventions (and could use a better name).


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:266-267
+  struct NolintTargetInfoEq {
+    bool operator()(const NolintTargetInfo &i1,
+                    const NolintTargetInfo &i2) const {
+      return i1.LineNumber == i2.LineNumber && i1.CheckName == i2.CheckName;
----------------
The parameter names don't match our coding conventions.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:273
+public:
+  NolintCommentsCollector(ClangTidyContext *Context)
+    : Context(Context) {}
----------------
Mark the constructor `explicit`?


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:276
+
+  using NolintMap = std::unordered_map<NolintTargetInfo,
+                                       NolintCommentInfo,
----------------
Is there a better LLVM ADT to use here?


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:316
+          Context->diag(NolintCheckName, NolintLoc,
+              "unknown check name '%0' is specified for %1 comment")
+                  << CheckName << CommentName;
----------------
I'd reword this slightly to: "unknown check name '%0' specified in %1 directive"


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:325-326
+        Context->diag(NolintCheckName, NolintLoc,
+            "%0 comment has an opening parenthesis and no closing one, "
+            "so all the diagnostics will be silenced for the line")
+                << CommentName;
----------------
How about: "unbalanced parentheses in %0 comment; all diagnostics silenced for this line" ?


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:381
 
+void ClangTidyContext::setPreprocessor(Preprocessor *preprocessor) {
+  if (preprocessor && isCheckEnabled(NolintCheckName)) {
----------------
Name does not match coding standard.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:424
+// This method is not const, because it fills cache on first demand.
+// It is possible to fill cache in constructor or make cache volatile
+// and make this method const, but they seem like worse solutions.
----------------
Making the cache volatile will have no impact on this.

Any reason not to make the cache `mutable`, however? That's quite common for implementation details.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:495
+  }
+  const SmallVector<StringRef, 1>& CheckNames = NolintParser.getCheckNames();
+  return llvm::find(CheckNames, CheckName) != CheckNames.end();
----------------
Formatting.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:566
+    // This is necessary for finding redundant NOLINT comments. Fill collection
+    // here, as there won't be another change if diagnostics is filtered.
+    DiagIDsToLineNumbers[Info.getID()].push_back(
----------------
s/change/chance?

Also: if diagnostics is filtered -> if diagnostics are filtered


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:839-840
+        case NolintCommentType::Nolint:
+          Message = "there is no diagnostics on this line, "
+                    "the NOLINT comment is redundant";
+          break;
----------------
I don't think the user is going to care about the distinction between no diagnostics being triggered and the expected diagnostic not being triggered. Also, it's dangerous to claim the lint comment is redundant because it's possible the user has NOLINT(foo, bar) and while foo is not triggered, bar still is. The NOLINT comment itself isn't redundant, it's that the check specified doesn't occur.

I would consolidate those scenarios into a single diagnostic: "expected diagnostic '%0' not generated" and "expected diagnostic '%0' not generated for the following line".

One concern I have with this functionality is: how should users silence a lint diagnostic that's target sensitive? e.g., a diagnostic that triggers based on the underlying type of size_t or the signedness of plain char. In that case, the diagnostic may trigger for some targets but not others, but on the targets where the diagnostic is not triggered, they now get a diagnostic they cannot silence. There should be a way to silence the "bad NOLINT" diagnostics.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:29
 class CompilerInstance;
+class Preprocessor;
 namespace ast_matchers {
----------------
Please add a newline after this.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:145
+  /// \brief Checks if a check with the specified name exists.
+  bool hasCheck(StringRef CheckName);
+
----------------
This function can be marked `const`.


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:205
 private:
-  // Calls setDiagnosticsEngine() and storeError().
+  // Calls setDiagnosticsEngine(), storeError() and getNolintCollector().
   friend class ClangTidyDiagnosticConsumer;
----------------
Comma after storeError()


================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:295
+  // separately to process NOLINT misuse.
+  std::unordered_map<unsigned, std::vector<unsigned>> DiagIDsToLineNumbers;
 };
----------------
Perhaps this would make more sense as an `llvm::IndexedMap` or other non-STL datatype?


https://reviews.llvm.org/D41326





More information about the cfe-commits mailing list