xgsa updated this revision to Diff 127267.
xgsa added a comment.
Review comments were applied.
https://reviews.llvm.org/D41326
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
test/clang-tidy/nolint-usage.cpp
test/clang-tidy/nolint.cpp
test/clang-tidy/nolintnextline.cpp
Index: test/clang-tidy/nolintnextline.cpp
===================================================================
--- test/clang-tidy/nolintnextline.cpp
+++ test/clang-tidy/nolintnextline.cpp
@@ -24,6 +24,18 @@
class C5 { C5(int i); };
+void f() {
+ int i;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+ // NOLINTNEXTLINE
+ int j;
+ // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+ int k;
+ // NOLINTNEXTLINE(clang-diagnostic-literal-conversion)
+ int l;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
+}
+
// NOLINTNEXTLINE
class D { D(int i); };
@@ -44,6 +56,6 @@
// NOLINTNEXTLINE
MACRO_NOARG
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 10 warnings (10 NOLINT)
-// RUN: %check_clang_tidy %s google-explicit-constructor %t --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable --
Index: test/clang-tidy/nolint.cpp
===================================================================
--- test/clang-tidy/nolint.cpp
+++ test/clang-tidy/nolint.cpp
@@ -30,6 +30,9 @@
int i;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
int j; // NOLINT
+ int k; // NOLINT(clang-diagnostic-unused-variable)
+ int l; // NOLINT(clang-diagnostic-literal-conversion)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'l' [clang-diagnostic-unused-variable]
}
#define MACRO(X) class X { X(int i); };
@@ -46,4 +49,4 @@
#define DOUBLE_MACRO MACRO(H) // NOLINT
DOUBLE_MACRO
-// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
+// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
Index: test/clang-tidy/nolint-usage.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/nolint-usage.cpp
@@ -0,0 +1,171 @@
+// RUN: %check_clang_tidy %s nolint-usage,google-explicit-constructor,misc-unused-parameters %t --
+
+// Case: NO_LINT on line with an error.
+class A1 { A1(int i); }; // NOLINT
+
+// Case: NO_LINT on line without errors.
+class A2 { explicit A2(int i); }; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line with an error on it.
+class A3 { A3(int i); }; // NOLINT(google-explicit-constructor)
+
+// Case: NO_LINT for the specific check on line with an error on another check.
+class A4 { A4(int i); }; // NOLINT(misc-unused-parameters)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: there is no diagnostics for the 'misc-unused-parameters' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for the specific check on line without errors.
+class A5 { explicit A5(int i); }; // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: there is no diagnostics for the 'google-explicit-constructor' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT for unknown check on line with an error on some check.
+class A6 { A6(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT for unknown check on line without errors.
+class A7 { explicit A7(int i); }; // NOLINT(unknown-check)
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: unknown check name 'unknown-check' is specified for NOLINT comment
+
+// Case: NO_LINT with not closed parenthesis without check names.
+class A8 { A8(int i); }; // NOLINT(
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT with not closed parenthesis with check names.
+class A9 { A9(int i); }; // NOLINT(unknown-check
+// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: NOLINT comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT with clang diagnostics
+int f() {
+ int i = 0; // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+ int j = 0; // NOLINT(clang-diagnostic-unused-variable)
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: there is no diagnostics for the 'clang-diagnostic-unused-variable' check on this line, the NOLINT comment is redundant
+ return i + j;
+}
+
+#define MACRO(X) class X { explicit X(int i); };
+
+// Case: NO_LINT on macro instantiation line
+MACRO(M1) // NOLINT
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT with the specific check on macro instantiation line
+MACRO(M2) // NOLINT(google-explicit-constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: there is no diagnostics for the 'google-explicit-constructor' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT on macro line
+#define MACRO2(X) class X { explicit X(int i); }; // NOLINT
+MACRO2(M3)
+// CHECK-MESSAGES: :[[@LINE-2]]:54: warning: there is no diagnostics on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT with the specific check on macro line
+#define MACRO3(X) class X { explicit X(int i); }; // NOLINT(google-explicit-constructor)
+MACRO(M4)
+// CHECK-MESSAGES: :[[@LINE-2]]:54: warning: there is no diagnostics for the 'google-explicit-constructor' check on this line, the NOLINT comment is redundant
+
+// Case: NO_LINT with nolint-usage on line without errors.
+class B1 { explicit B1(int i); }; // NOLINT(nolint-usage)
+
+// Case: NO_LINT with the specific check and nolint-usage on line without errors.
+class B2 { explicit B2(int i); }; // NOLINT(google-explicit-constructor, nolint-usage)
+
+// Case: NO_LINT with the unknown check and nolint-usage on line without errors.
+class B3 { explicit B3(int i); }; // NOLINT(unknown-check, nolint-usage)
+
+
+// Case: NO_LINT_NEXT_LINE before line with an error.
+// NOLINTNEXTLINE
+class C1 { C1(int i); };
+
+// Case: NO_LINT_NEXT_LINE before line without errors.
+// NOLINTNEXTLINE
+class C2 { explicit C2(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there is no diagnostics on the following line, the NOLINTNEXTLINE comment is redundant
+
+// Case: NO_LINT_NEXT_LINE for the specific check before line with an error on it.
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// Case: NO_LINT_NEXT_LINE for the specific check before line with an error on another check.
+// NOLINTNEXTLINE(misc-unused-parameters)
+class C4 { C4(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there is no diagnostics for the 'misc-unused-parameters' check on the following line, the NOLINTNEXTLINE comment is redundant
+// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: single-argument constructors must be marked explicit
+
+// Case: NO_LINT_NEXT_LINE for the specific check before line without errors.
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C5 { explicit C5(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there is no diagnostics for the 'google-explicit-constructor' check on the following line, the NOLINTNEXTLINE comment is redundant
+
+// Case: NO_LINT_NEXT_LINE for unknown check before line with an error on some check.
+// NOLINTNEXTLINE(unknown-check)
+class C6 { C6(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unknown check name 'unknown-check' is specified for NOLINTNEXTLINE comment
+// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: single-argument constructors must be marked explicit
+
+// Case: NO_LINT_NEXT_LINE for unknown check before line without errors.
+// NOLINTNEXTLINE(unknown-check)
+class C7 { explicit C7(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: unknown check name 'unknown-check' is specified for NOLINTNEXTLINE comment
+
+// Case: NO_LINT_NEXT_LINE with not closed parenthesis without check names.
+// NOLINTNEXTLINE(
+class C8 { C8(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: NOLINTNEXTLINE comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT_NEXT_LINE with not closed parenthesis with check names.
+// NOLINTNEXTLINE(unknown-check
+class C9 { C9(int i); };
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: NOLINTNEXTLINE comment has an opening parenthesis and no closing one, so all the diagnostics will be silenced for the line
+
+// Case: NO_LINT_NEXT_LINE with clang diagnostics
+int g() {
+ // NOLINTNEXTLINE
+ int i = 0;
+// CHECK-MESSAGES: :[[@LINE-2]]:6: warning: there is no diagnostics on the following line, the NOLINTNEXTLINE comment is redundant
+ // NOLINTNEXTLINE(clang-diagnostic-unused-variable)
+ int j = 0;
+// CHECK-MESSAGES: :[[@LINE-2]]:6: warning: there is no diagnostics for the 'clang-diagnostic-unused-variable' check on the following line, the NOLINTNEXTLINE comment is redundant
+ return i + j;
+}
+
+#define MACRO(X) class X { explicit X(int i); };
+
+// Case: NO_LINT_NEXT_LINE before macro instantiation line
+// NOLINTNEXTLINE
+MACRO(MM1)
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there is no diagnostics on the following line, the NOLINTNEXTLINE comment is redundant
+
+// Case: NO_LINT_NEXT_LINE with the specific check before macro instantiation line
+// NOLINTNEXTLINE(google-explicit-constructor)
+MACRO(MM2)
+// CHECK-MESSAGES: :[[@LINE-2]]:4: warning: there is no diagnostics for the 'google-explicit-constructor' check on the following line, the NOLINTNEXTLINE comment is redundant
+
+// Case: NO_LINT_NEXT_LINE before macro line
+// NOLINTNEXTLINE
+#define MACRO2(X) class X { explicit X(int i); };
+MACRO2(MM3)
+// CHECK-MESSAGES: :[[@LINE-3]]:4: warning: there is no diagnostics on the following line, the NOLINTNEXTLINE comment is redundant
+
+// Case: NO_LINT_NEXT_LINE with the specific check before macro line
+// NOLINTNEXTLINE(google-explicit-constructor)
+#define MACRO3(X) class X { explicit X(int i); };
+MACRO(MM4)
+// CHECK-MESSAGES: :[[@LINE-3]]:4: warning: there is no diagnostics for the 'google-explicit-constructor' check on the following line, the NOLINTNEXTLINE comment is redundant
+
+// Case: NO_LINT_NEXT_LINE with nolint-usage before line without errors.
+// NOLINTNEXTLINE(nolint-usage)
+// NOLINTNEXTLINE
+class D1 { explicit D1(int i); };
+
+// Case: NO_LINT_NEXT_LINE with the specific check and nolint-usage before line without errors.
+// NOLINTNEXTLINE(nolint-usage)
+// NOLINTNEXTLINE(google-explicit-constructor)
+class D2 { explicit D2(int i); };
+
+// Case: NO_LINT_NEXT_LINE with the unknown check and nolint-usage before line without errors.
+// NOLINTNEXTLINE(nolint-usage)
+// NOLINTNEXTLINE(unknown-check)
+class D3 { explicit D3(int i); };
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -19,19 +19,23 @@
#include "llvm/ADT/StringMap.h"
#include "llvm/Support/Regex.h"
#include "llvm/Support/Timer.h"
+#include <unordered_map>
+#include <vector>
namespace clang {
class ASTContext;
class CompilerInstance;
+class Preprocessor;
namespace ast_matchers {
class MatchFinder;
}
namespace tooling {
class CompilationDatabase;
}
namespace tidy {
+class NolintCommentsCollector;
/// \brief A detected error complete with information to display diagnostic and
/// automatic fix.
@@ -131,13 +135,24 @@
/// \brief Sets ASTContext for the current translation unit.
void setASTContext(ASTContext *Context);
+ /// \brief Subscribes the context to the preprocessor events if necessary.
+ void setPreprocessor(Preprocessor *preprocessor);
+
/// \brief Gets the language options from the AST context.
const LangOptions &getLangOpts() const { return LangOpts; }
+ /// \brief Checks if a check with the specified name exists.
+ bool hasCheck(StringRef CheckName);
+
/// \brief Returns the name of the clang-tidy check which produced this
/// diagnostic ID.
StringRef getCheckName(unsigned DiagnosticID) const;
+ /// \brief Returns the name of diagnostics as it will be output in report
+ /// for the specified diagnostic ID. It can be either a check name or
+ /// clang diagnostics with "clang-diagnostic-" prefix.
+ std::string getDiagnosticsName(unsigned DiagnosticID) const;
+
/// \brief Returns \c true if the check is enabled for the \c CurrentFile.
///
/// The \c CurrentFile can be changed using \c setCurrentFile.
@@ -187,7 +202,7 @@
}
private:
- // Calls setDiagnosticsEngine() and storeError().
+ // Calls setDiagnosticsEngine(), storeError() and getNolintCollector().
friend class ClangTidyDiagnosticConsumer;
friend class ClangTidyPluginAction;
@@ -198,6 +213,9 @@
/// \brief Store an \p Error.
void storeError(const ClangTidyError &Error);
+ // \brief Provides access to the \c NOLINT/NOLINTNEXTLINE info collector.
+ NolintCommentsCollector &getNolintCollector();
+
std::vector<ClangTidyError> Errors;
DiagnosticsEngine *DiagEngine;
std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
@@ -217,6 +235,9 @@
llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
ProfileData *Profile;
+
+ std::unique_ptr<NolintCommentsCollector> NolintCollector;
+ std::vector<std::string> AllCheckNamesCache;
};
/// \brief A diagnostic consumer that turns each \c Diagnostic into a
@@ -241,8 +262,17 @@
private:
void finalizeLastError();
+ bool isLineMarkedWithNolintInMacro(SourceManager &SM, SourceLocation Loc,
+ StringRef CheckName);
+ bool lineIsMarkedWithNolint(SourceManager &SM, SourceLocation Loc,
+ StringRef CheckName);
+ bool isNolintFound(StringRef NolintDirectiveText, StringRef Line,
+ StringRef CheckName);
+
void removeIncompatibleErrors(SmallVectorImpl<ClangTidyError> &Errors) const;
+ void reportRedundantNolintComments();
+
/// \brief Returns the \c HeaderFilter constructed for the options set in the
/// context.
llvm::Regex *getHeaderFilter();
@@ -257,9 +287,12 @@
std::unique_ptr<DiagnosticsEngine> Diags;
SmallVector<ClangTidyError, 8> Errors;
std::unique_ptr<llvm::Regex> HeaderFilter;
- bool LastErrorRelatesToUserCode;
- bool LastErrorPassesLineFilter;
- bool LastErrorWasIgnored;
+ bool LastErrorRelatesToUserCode = false;
+ bool LastErrorPassesLineFilter = false;
+ bool LastErrorWasIgnored = false;
+ // Unfortunately, line numbers are not stored in Errors, so gather them
+ // separately to process NOLINT misuse.
+ std::unordered_map<unsigned, std::vector<unsigned>> DiagIDsToLineNumbers;
};
} // end namespace tidy
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -18,11 +18,17 @@
#include "ClangTidyDiagnosticConsumer.h"
#include "ClangTidyOptions.h"
+#include "ClangTidy.h"
+#include "ClangTidyModule.h"
#include "clang/AST/ASTDiagnostic.h"
#include "clang/Basic/DiagnosticOptions.h"
#include "clang/Frontend/DiagnosticRenderer.h"
+#include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include <algorithm>
#include <tuple>
#include <vector>
using namespace clang;
@@ -105,6 +111,68 @@
private:
ClangTidyError &Error;
};
+
+const StringRef ClangDiagnosticsPrefix = "clang-diagnostic-";
+const StringRef NolintCheckName = "nolint-usage";
+enum class NolintCommentType : uint8_t {
+ Nolint,
+ NolintNextLine,
+};
+const StringRef NolintComment = "NOLINT";
+const StringRef NolintNextLineComment = "NOLINTNEXTLINE";
+
+class NolintCommentParser {
+public:
+ NolintCommentParser(StringRef NolintDirectiveText, StringRef Line) {
+ if ((NolintIndex = Line.find(NolintDirectiveText)) != StringRef::npos) {
+ size_t BracketIndex = NolintIndex + NolintDirectiveText.size();
+ // Check if the specific checks are specified in brackets.
+ if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+ ++BracketIndex;
+ const size_t BracketEndIndex = Line.find(')', BracketIndex);
+ if ((ClosingBracketFound = BracketEndIndex != StringRef::npos)) {
+ StringRef ChecksStr =
+ Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
+ // Allow disabling all the checks with "*".
+ if (ChecksStr != "*") {
+ // Allow specifying a few check names, delimited with comma.
+ ChecksStr.split(CheckNames, ',', -1, false);
+ llvm::transform(CheckNames, CheckNames.begin(),
+ [](StringRef S) { return S.trim(); });
+ }
+ }
+ }
+ }
+ }
+
+ bool isNolintFound() const {
+ return NolintIndex != StringRef::npos;
+ }
+
+ size_t getNolintIndex() const {
+ assert(isNolintFound());
+ return NolintIndex;
+ }
+
+ bool isAppliedToAllChecks() const {
+ return CheckNames.empty();
+ }
+
+ const SmallVector<StringRef, 1>& getCheckNames() const {
+ assert(isNolintFound());
+ assert(!isAppliedToAllChecks());
+ return CheckNames;
+ }
+
+ bool isClosingBracketFound() const {
+ return ClosingBracketFound;
+ }
+
+private:
+ SmallVector<StringRef, 1> CheckNames;
+ size_t NolintIndex = 0;
+ bool ClosingBracketFound = true;
+};
} // end anonymous namespace
ClangTidyError::ClangTidyError(StringRef CheckName,
@@ -176,10 +244,102 @@
llvm::StringMap<Tristate> Cache;
};
+class clang::tidy::NolintCommentsCollector : public CommentHandler {
+public:
+ struct NolintTargetInfo {
+ unsigned LineNumber;
+ StringRef CheckName;
+ };
+ struct NolintCommentInfo {
+ SourceLocation Location;
+ NolintCommentType Type;
+ };
+ static const StringRef AnyCheck;
+
+private:
+ struct NolintTargetInfoHasher {
+ size_t operator() (const NolintTargetInfo &i) const {
+ return llvm::hash_combine(i.LineNumber, i.CheckName);
+ }
+ };
+ struct NolintTargetInfoEq {
+ bool operator()(const NolintTargetInfo &i1,
+ const NolintTargetInfo &i2) const {
+ return i1.LineNumber == i2.LineNumber && i1.CheckName == i2.CheckName;
+ }
+ };
+
+public:
+ NolintCommentsCollector(ClangTidyContext *Context)
+ : Context(Context) {}
+
+ using NolintMap = std::unordered_map<NolintTargetInfo,
+ NolintCommentInfo,
+ NolintTargetInfoHasher,
+ NolintTargetInfoEq>;
+ NolintMap takeNolintMap() {
+ NolintMap result;
+ std::swap(result, NolintComments);
+ return result;
+ }
+
+private:
+ bool HandleComment(Preprocessor &PP, SourceRange Range) override {
+ SourceManager &SrcMgr = PP.getSourceManager();
+ StringRef Text = Lexer::getSourceText(CharSourceRange::getCharRange(Range),
+ SrcMgr, PP.getLangOpts());
+ // Search for NOLINTNEXTLINE first (as NOLINT could be a part of it).
+ StringRef CommentName = NolintNextLineComment;
+ NolintCommentType CommentType = NolintCommentType::NolintNextLine;
+ unsigned LineLocationOffset = 1;
+ NolintCommentParser NolintParser(CommentName, Text);
+ if (!NolintParser.isNolintFound()) {
+ // Search for NOLINT otherwise.
+ CommentName = NolintComment;
+ CommentType = NolintCommentType::Nolint;
+ LineLocationOffset = 0;
+ NolintParser = NolintCommentParser(CommentName, Text);
+ }
+ if (NolintParser.isNolintFound()) {
+ SourceLocation NolintLoc =
+ Range.getBegin().getLocWithOffset(NolintParser.getNolintIndex());
+ unsigned LineNo = SrcMgr.getExpansionLineNumber(Range.getBegin()) + LineLocationOffset;
+ static const SmallVector<StringRef, 1> AllChecks = { AnyCheck };
+ for (StringRef CheckName : NolintParser.isAppliedToAllChecks() ?
+ AllChecks : NolintParser.getCheckNames()) {
+ // Don't report diagnostics to itself.
+ if (CheckName == NolintCheckName)
+ continue;
+ if (!CheckName.empty() && !Context->hasCheck(CheckName) &&
+ !CheckName.startswith(ClangDiagnosticsPrefix)) {
+ Context->diag(NolintCheckName, NolintLoc,
+ "unknown check name '%0' is specified for %1 comment")
+ << CheckName << CommentName;
+ } else {
+ NolintComments.emplace(NolintTargetInfo{LineNo, CheckName},
+ NolintCommentInfo{NolintLoc, CommentType});
+ }
+ }
+ if (!NolintParser.isClosingBracketFound()) {
+ 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;
+ }
+ }
+ return false;
+ }
+
+private:
+ NolintMap NolintComments;
+ ClangTidyContext *Context;
+};
+const StringRef NolintCommentsCollector::AnyCheck;
+
ClangTidyContext::ClangTidyContext(
std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider)
- : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
- Profile(nullptr) {
+ : OptionsProvider(std::move(OptionsProvider)),
+ NolintCollector(llvm::make_unique<NolintCommentsCollector>(this)) {
// Before the first translation unit we can get errors related to command-line
// parsing, use empty string for the file name in this case.
setCurrentFile("");
@@ -218,6 +378,12 @@
LangOpts = Context->getLangOpts();
}
+void ClangTidyContext::setPreprocessor(Preprocessor *preprocessor) {
+ if (preprocessor && isCheckEnabled(NolintCheckName)) {
+ preprocessor->addCommentHandler(NolintCollector.get());
+ }
+}
+
const ClangTidyGlobalOptions &ClangTidyContext::getGlobalOptions() const {
return OptionsProvider->getGlobalOptions();
}
@@ -250,14 +416,39 @@
Errors.push_back(Error);
}
+NolintCommentsCollector &ClangTidyContext::getNolintCollector() {
+ return *NolintCollector;
+}
+
+// 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.
+bool ClangTidyContext::hasCheck(StringRef CheckName) {
+ if (AllCheckNamesCache.empty()) {
+ ClangTidyASTConsumerFactory Factory(*this);
+ AllCheckNamesCache = Factory.getCheckNames();
+ assert(std::is_sorted(AllCheckNamesCache.begin(), AllCheckNamesCache.end()));
+ }
+ return std::binary_search(AllCheckNamesCache.begin(),
+ AllCheckNamesCache.end(), CheckName);
+}
+
StringRef ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
llvm::DenseMap<unsigned, std::string>::const_iterator I =
CheckNamesByDiagnosticID.find(DiagnosticID);
if (I != CheckNamesByDiagnosticID.end())
return I->second;
return "";
}
+std::string ClangTidyContext::getDiagnosticsName(unsigned DiagnosticID) const {
+ StringRef WarningOption =
+ DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID);
+ return !WarningOption.empty()
+ ? (ClangDiagnosticsPrefix + WarningOption).str()
+ : getCheckName(DiagnosticID).str();
+}
+
ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
ClangTidyContext &Ctx, bool RemoveIncompatibleErrors)
: Context(Ctx), RemoveIncompatibleErrors(RemoveIncompatibleErrors),
@@ -291,38 +482,25 @@
LastErrorPassesLineFilter = false;
}
-static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,
- unsigned DiagID, const ClangTidyContext &Context) {
- const size_t NolintIndex = Line.find(NolintDirectiveText);
- if (NolintIndex == StringRef::npos)
+bool ClangTidyDiagnosticConsumer::isNolintFound(StringRef NolintDirectiveText,
+ StringRef Line,
+ StringRef CheckName) {
+ NolintCommentParser NolintParser(NolintDirectiveText, Line);
+ if (!NolintParser.isNolintFound()) {
return false;
-
- size_t BracketIndex = NolintIndex + NolintDirectiveText.size();
- // Check if the specific checks are specified in brackets.
- if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
- ++BracketIndex;
- const size_t BracketEndIndex = Line.find(')', BracketIndex);
- if (BracketEndIndex != StringRef::npos) {
- StringRef ChecksStr =
- Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
- // Allow disabling all the checks with "*".
- if (ChecksStr != "*") {
- StringRef CheckName = Context.getCheckName(DiagID);
- // Allow specifying a few check names, delimited with comma.
- SmallVector<StringRef, 1> Checks;
- ChecksStr.split(Checks, ',', -1, false);
- llvm::transform(Checks, Checks.begin(),
- [](StringRef S) { return S.trim(); });
- return llvm::find(Checks, CheckName) != Checks.end();
- }
- }
}
- return true;
+ if (NolintParser.isAppliedToAllChecks()) {
+ // NOLINT without check name shouldn't block NolintCheck itself, because
+ // otherwise there will never be diagnostics on "// NOLINT".
+ return CheckName != NolintCheckName;
+ }
+ const SmallVector<StringRef, 1>& CheckNames = NolintParser.getCheckNames();
+ return llvm::find(CheckNames, CheckName) != CheckNames.end();
}
-static bool LineIsMarkedWithNOLINT(SourceManager &SM, SourceLocation Loc,
- unsigned DiagID,
- const ClangTidyContext &Context) {
+bool ClangTidyDiagnosticConsumer::lineIsMarkedWithNolint(SourceManager &SM,
+ SourceLocation Loc,
+ StringRef CheckName) {
bool Invalid;
const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
if (Invalid)
@@ -333,7 +511,7 @@
while (*P != '\0' && *P != '\r' && *P != '\n')
++P;
StringRef RestOfLine(CharacterData, P - CharacterData + 1);
- if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
+ if (isNolintFound(NolintComment, RestOfLine, CheckName))
return true;
// Check if there's a NOLINTNEXTLINE on the previous line.
@@ -360,17 +538,16 @@
--P;
RestOfLine = StringRef(P, LineEnd - P + 1);
- if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLine, DiagID, Context))
+ if (isNolintFound(NolintNextLineComment, RestOfLine, CheckName))
return true;
return false;
}
-static bool LineIsMarkedWithNOLINTinMacro(SourceManager &SM, SourceLocation Loc,
- unsigned DiagID,
- const ClangTidyContext &Context) {
+bool ClangTidyDiagnosticConsumer::isLineMarkedWithNolintInMacro(
+ SourceManager &SM, SourceLocation Loc, StringRef CheckName) {
while (true) {
- if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
+ if (lineIsMarkedWithNolint(SM, Loc, CheckName))
return true;
if (!Loc.isMacroID())
return false;
@@ -384,33 +561,31 @@
if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
return;
- if (Info.getLocation().isValid() && DiagLevel != DiagnosticsEngine::Error &&
- DiagLevel != DiagnosticsEngine::Fatal &&
- LineIsMarkedWithNOLINTinMacro(Diags->getSourceManager(),
- Info.getLocation(), Info.getID(),
- Context)) {
- ++Context.Stats.ErrorsIgnoredNOLINT;
- // Ignored a warning, should ignore related notes as well
- LastErrorWasIgnored = true;
- return;
+ std::string CheckName = Context.getDiagnosticsName(Info.getID());
+ SourceLocation DiagLocation = Info.getLocation();
+ if (DiagLocation.isValid()) {
+ // 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(
+ Info.getSourceManager().getExpansionLineNumber(DiagLocation));
+
+ if (DiagLevel != DiagnosticsEngine::Error &&
+ DiagLevel != DiagnosticsEngine::Fatal &&
+ isLineMarkedWithNolintInMacro(Diags->getSourceManager(), DiagLocation,
+ CheckName)) {
+ ++Context.Stats.ErrorsIgnoredNOLINT;
+ // Ignored a warning, should ignore related notes as well.
+ LastErrorWasIgnored = true;
+ return;
+ }
}
LastErrorWasIgnored = false;
// Count warnings/errors.
DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
- if (DiagLevel == DiagnosticsEngine::Note) {
- assert(!Errors.empty() &&
- "A diagnostic note can only be appended to a message.");
- } else {
+ if (DiagLevel != DiagnosticsEngine::Note) {
finalizeLastError();
- StringRef WarningOption =
- Context.DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(
- Info.getID());
- std::string CheckName = !WarningOption.empty()
- ? ("clang-diagnostic-" + WarningOption).str()
- : Context.getCheckName(Info.getID()).str();
-
if (CheckName.empty()) {
// This is a compiler diagnostic without a warning option. Assign check
// name based on its level.
@@ -441,6 +616,9 @@
Context.treatAsError(CheckName);
Errors.emplace_back(CheckName, Level, Context.getCurrentBuildDirectory(),
IsWarningAsError);
+ } else {
+ assert(!Errors.empty() &&
+ "A diagnostic note can only be appended to a message.");
}
ClangTidyDiagnosticRenderer Converter(
@@ -641,6 +819,51 @@
}
}
+void ClangTidyDiagnosticConsumer::reportRedundantNolintComments() {
+ NolintCommentsCollector::NolintMap NolintMap =
+ Context.getNolintCollector().takeNolintMap();
+ if (NolintMap.empty())
+ return;
+ for (const auto &DiagIDToLineNumbers : DiagIDsToLineNumbers) {
+ std::string CheckName = Context.getDiagnosticsName(DiagIDToLineNumbers.first);
+ if (CheckName.empty())
+ continue;
+ for (unsigned LineNumber : DiagIDToLineNumbers.second) {
+ NolintMap.erase({LineNumber, CheckName});
+ NolintMap.erase({LineNumber, NolintCommentsCollector::AnyCheck});
+ }
+ }
+ for (const auto &NolintEntry : NolintMap) {
+ StringRef Message;
+ if (NolintEntry.first.CheckName == NolintCommentsCollector::AnyCheck) {
+ switch (NolintEntry.second.Type) {
+ case NolintCommentType::Nolint:
+ Message = "there is no diagnostics on this line, "
+ "the NOLINT comment is redundant";
+ break;
+ case NolintCommentType::NolintNextLine:
+ Message = "there is no diagnostics on the following line, "
+ "the NOLINTNEXTLINE comment is redundant";
+ break;
+ }
+ Context.diag(NolintCheckName, NolintEntry.second.Location, Message);
+ } else {
+ switch (NolintEntry.second.Type) {
+ case NolintCommentType::Nolint:
+ Message = "there is no diagnostics for the '%0' check on this line, "
+ "the NOLINT comment is redundant";
+ break;
+ case NolintCommentType::NolintNextLine:
+ Message = "there is no diagnostics for the '%0' check on the "
+ "following line, the NOLINTNEXTLINE comment is redundant";
+ break;
+ }
+ Context.diag(NolintCheckName, NolintEntry.second.Location, Message)
+ << NolintEntry.first.CheckName;
+ }
+ }
+}
+
namespace {
struct LessClangTidyError {
bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
@@ -661,6 +884,7 @@
// Flushes the internal diagnostics buffer to the ClangTidyContext.
void ClangTidyDiagnosticConsumer::finish() {
+ reportRedundantNolintComments();
finalizeLastError();
std::sort(Errors.begin(), Errors.end(), LessClangTidyError());
@@ -673,4 +897,5 @@
for (const ClangTidyError &Error : Errors)
Context.storeError(Error);
Errors.clear();
+ DiagIDsToLineNumbers.clear();
}
Index: clang-tidy/ClangTidy.cpp
===================================================================
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -340,6 +340,7 @@
Context.setSourceManager(&Compiler.getSourceManager());
Context.setCurrentFile(File);
Context.setASTContext(&Compiler.getASTContext());
+ Context.setPreprocessor(&Compiler.getPreprocessor());
auto WorkingDir = Compiler.getSourceManager()
.getFileManager()
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
