carlosgalvezp created this revision.
carlosgalvezp added a reviewer: aaron.ballman.
Herald added subscribers: jeroen.dobbelaere, kbarton, xazax.hun, nemanjai.
carlosgalvezp requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Find "perfect aliases" and remove them from the list
of checks to run. On large codebases, this can save
up to 30% runtime, which is not negligible.
A "perfect alias" is an alias that:
- Is of the same check class as the primary check.
- Has all configuration options the same as the primary check.
NOTE TO REVIEWERS:
This is a WIP, quick and dirty PoC, so the code is not pretty,
well structured nor optimal.
The intention is to make this feature opt-in via config/CLI.
I.e. turned off by default -> should not break things.
Before polishing the patch I'd like to get feedback about
whether this is the desired direction, and in general if
I'm putting the pieces in the right places.
DESIGN GOALS:
- No impact on check developers, other than having to register the check via
the new macro instead of the regular function.
- Only perfect aliases to be removed.
MISSING BITS/REQUIREMENTS FROM THE RFC:
- It would be desirable to still keep the diagnostic/fixit for the alias
checks. I don't know how to implement this in practice. It would require each
check to call the diag() function in a loop for all alias checks. This would be
very disruptive for check developers. I cannot implement this in the
ClangTidyCheck class because the diag() function returns an object, so I can't
put a loop there.
- Ideally, it would be nice to show "CACHED - 0 seconds" or "SKIPPED - 0
seconds" in the profiling output (currently, the alias check just doesn't show
up). Again, I don't know how to implement this feature in a non-intrusive way
that makes sense.
Suggestions are very welcome for these two bits! Otherwise I am
personally happy to skip them.
Current working test:
clang-tidy -checks=-*,*-c-arrays,*-in-classes
The result is:
- Only one of the 3 "c-arrays" check is run (the primary check), because the
other 2 checks are perfect aliases.
- Both of the "-in-classes" checks (cppcoreguidelines, misc) are run, because
they have different configuration options.
https://reviews.llvm.org/D114317
Files:
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/ClangTidyCheck.h
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/clang-tidy/ClangTidyModule.h
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp
clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
Index: clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -51,7 +51,8 @@
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
CheckFactories.registerCheck<AvoidBindCheck>("modernize-avoid-bind");
- CheckFactories.registerCheck<AvoidCArraysCheck>("modernize-avoid-c-arrays");
+ CLANG_TIDY_REGISTER_CHECK(CheckFactories, AvoidCArraysCheck,
+ "modernize-avoid-c-arrays");
CheckFactories.registerCheck<ConcatNestedNamespacesCheck>(
"modernize-concat-nested-namespaces");
CheckFactories.registerCheck<DeprecatedHeadersCheck>(
Index: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -42,8 +42,9 @@
CheckFactories.registerCheck<NoRecursionCheck>("misc-no-recursion");
CheckFactories.registerCheck<NonCopyableObjectsCheck>(
"misc-non-copyable-objects");
- CheckFactories.registerCheck<NonPrivateMemberVariablesInClassesCheck>(
- "misc-non-private-member-variables-in-classes");
+ CLANG_TIDY_REGISTER_CHECK(CheckFactories,
+ NonPrivateMemberVariablesInClassesCheck,
+ "misc-non-private-member-variables-in-classes");
CheckFactories.registerCheck<RedundantExpressionCheck>(
"misc-redundant-expression");
CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert");
Index: clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp
+++ clang-tools-extra/clang-tidy/hicpp/HICPPTidyModule.cpp
@@ -48,8 +48,8 @@
class HICPPModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
- CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
- "hicpp-avoid-c-arrays");
+ CLANG_TIDY_REGISTER_CHECK(CheckFactories, modernize::AvoidCArraysCheck,
+ "hicpp-avoid-c-arrays");
CheckFactories.registerCheck<cppcoreguidelines::AvoidGotoCheck>(
"hicpp-avoid-goto");
CheckFactories.registerCheck<readability::BracesAroundStatementsCheck>(
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -45,8 +45,8 @@
class CppCoreGuidelinesModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
- CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
- "cppcoreguidelines-avoid-c-arrays");
+ CLANG_TIDY_REGISTER_CHECK(CheckFactories, modernize::AvoidCArraysCheck,
+ "cppcoreguidelines-avoid-c-arrays");
CheckFactories.registerCheck<AvoidGotoCheck>(
"cppcoreguidelines-avoid-goto");
CheckFactories.registerCheck<readability::MagicNumbersCheck>(
@@ -64,7 +64,8 @@
CheckFactories.registerCheck<NarrowingConversionsCheck>(
"cppcoreguidelines-narrowing-conversions");
CheckFactories.registerCheck<NoMallocCheck>("cppcoreguidelines-no-malloc");
- CheckFactories.registerCheck<misc::NonPrivateMemberVariablesInClassesCheck>(
+ CLANG_TIDY_REGISTER_CHECK(
+ CheckFactories, misc::NonPrivateMemberVariablesInClassesCheck,
"cppcoreguidelines-non-private-member-variables-in-classes");
CheckFactories.registerCheck<OwningMemoryCheck>(
"cppcoreguidelines-owning-memory");
Index: clang-tools-extra/clang-tidy/ClangTidyModule.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyModule.h
+++ clang-tools-extra/clang-tidy/ClangTidyModule.h
@@ -9,6 +9,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYMODULE_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYMODULE_H
+#include "ClangTidyDiagnosticConsumer.h"
#include "ClangTidyOptions.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
@@ -21,6 +22,9 @@
class ClangTidyCheck;
class ClangTidyContext;
+#define CLANG_TIDY_REGISTER_CHECK(Factory, CheckType, CheckName) \
+ Factory.registerCheck<CheckType>(CheckName, #CheckType)
+
/// A collection of \c ClangTidyCheckFactory instances.
///
/// All clang-tidy modules register their check factories with an instance of
@@ -35,6 +39,9 @@
/// For all checks that have default constructors, use \c registerCheck.
void registerCheckFactory(llvm::StringRef Name, CheckFactory Factory);
+ void registerCheckFactory(llvm::StringRef Name, llvm::StringRef Type,
+ CheckFactory Factory);
+
/// Registers the \c CheckType with the name \p Name.
///
/// This method should be used for all \c ClangTidyChecks that don't require
@@ -63,6 +70,38 @@
});
}
+ template <typename CheckType>
+ void registerCheck(llvm::StringRef CheckName, llvm::StringRef CheckTypeStr) {
+ registerCheckFactory(
+ CheckName, [CheckName, CheckTypeStr](llvm::StringRef Name,
+ ClangTidyContext *Context) {
+ StringRef CheckTypeStrUnqualified = CheckTypeStr;
+ bool isAliasCheck = false;
+ if (CheckTypeStr.contains("::")) {
+ isAliasCheck = true;
+ CheckTypeStrUnqualified =
+ CheckTypeStr.substr(CheckTypeStr.rfind("::") + 2);
+ }
+
+ Context->MapCheckToCheckType[CheckName] = CheckTypeStrUnqualified;
+
+ // If the check is an alias, we push it to the end of the list of
+ // checks associated with the check name
+ if (isAliasCheck) {
+ Context->MapCheckTypeToChecks[CheckTypeStrUnqualified].push_back(
+ CheckName);
+ }
+ // Otherwise, if we found a primary check, put it first in the list
+ else {
+ Context->MapCheckTypeToChecks[CheckTypeStrUnqualified].insert(
+ Context->MapCheckTypeToChecks[CheckTypeStrUnqualified].begin(),
+ CheckName);
+ }
+
+ return std::make_unique<CheckType>(Name, Context);
+ });
+ }
+
/// Create instances of checks that are enabled.
std::vector<std::unique_ptr<ClangTidyCheck>>
createChecks(ClangTidyContext *Context);
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -182,6 +182,13 @@
DiagEngine->getDiagnosticIDs()->getDescription(DiagnosticID)));
}
+ // { "cppcoreguidelines-avoid-c-arrays": "AvoidCArraysCheck" }
+ std::map<StringRef, StringRef> MapCheckToCheckType;
+
+ // { "AvoidCArraysCheck": ["modernize-avoid-c-arrays",
+ // "cppcoreguidelines-avoid-c-arrays"]}
+ std::map<StringRef, std::vector<StringRef>> MapCheckTypeToChecks;
+
private:
// Writes to Stats.
friend class ClangTidyDiagnosticConsumer;
Index: clang-tools-extra/clang-tidy/ClangTidyCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/ClangTidyCheck.h
@@ -125,6 +125,13 @@
configurationDiag(StringRef Description,
DiagnosticIDs::Level Level = DiagnosticIDs::Warning) const;
+ bool isAlias(ClangTidyContext *Context) {
+ StringRef CheckType = Context->MapCheckToCheckType[getID()];
+ return std::find(Context->MapCheckTypeToChecks[CheckType].begin(),
+ Context->MapCheckTypeToChecks[CheckType].end(), getID()) !=
+ Context->MapCheckTypeToChecks[CheckType].begin();
+ }
+
/// Should store all options supported by this check with their
/// current values or default values for options that haven't been overridden.
///
@@ -132,6 +139,8 @@
/// whether it has the default value or it has been overridden.
virtual void storeOptions(ClangTidyOptions::OptionMap &Options) {}
+ StringRef getID() const override { return CheckName; }
+
/// Provides access to the ``ClangTidyCheck`` options via check-local
/// names.
///
@@ -408,7 +417,6 @@
private:
void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
- StringRef getID() const override { return CheckName; }
std::string CheckName;
ClangTidyContext *Context;
Index: clang-tools-extra/clang-tidy/ClangTidy.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -41,6 +41,7 @@
#include "clang/Tooling/Tooling.h"
#include "llvm/Support/Process.h"
#include <algorithm>
+#include <iostream>
#include <utility>
#if CLANG_TIDY_ENABLE_STATIC_ANALYZER
@@ -408,6 +409,53 @@
return !Check->isLanguageVersionSupported(Context.getLangOpts());
});
+ ClangTidyOptions::OptionMap AllCheckOptions = getCheckOptions();
+
+ llvm::erase_if(Checks, [&](std::unique_ptr<ClangTidyCheck> &Check) {
+ // Do not delete primary checks
+ if (!Check->isAlias(&Context)) {
+ return false;
+ }
+
+ // Get name of primary check
+ StringRef CheckName = Check->getID();
+ StringRef CheckType = Context.MapCheckToCheckType[CheckName];
+ StringRef PrimaryCheckName =
+ Context.MapCheckTypeToChecks[CheckType].front();
+
+ // If the check is an alias, delete it only if it's a *perfect* alias,
+ // i.e. it has the same options as the primary check
+ for (auto const &Option : AllCheckOptions) {
+ StringRef Key = Option.getKey();
+ StringRef Value = Option.getValue().Value;
+
+ if (Key.startswith(CheckName)) {
+ StringRef OptionName = Key.substr(CheckName.size() + 1);
+
+ std::string PrimaryCheckOptionName =
+ PrimaryCheckName.str() + "." + OptionName.str();
+ StringRef PrimaryCheckValue =
+ AllCheckOptions[StringRef(PrimaryCheckOptionName)].Value;
+
+ // Found one option that is different from primary check -> keep
+ if (PrimaryCheckValue != Value) {
+ std::cout << "Alias check \"" << CheckName.str() << "\" of \""
+ << PrimaryCheckName.str() << "\""
+ << " has a different option: " << Key.str() << "="
+ << Value.str() << " vs " << PrimaryCheckOptionName << "="
+ << PrimaryCheckValue.str()
+ << ". Keeping alias check as it's not a perfect alias"
+ << std::endl;
+ return false;
+ }
+ }
+ }
+
+ // We have a perfect alias -> remove
+ std::cout << "Removing perfect alias " << Check->getID().str() << std::endl;
+ return true;
+ });
+
ast_matchers::MatchFinder::MatchFinderOptions FinderOptions;
std::unique_ptr<ClangTidyProfiling> Profiling;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
