njames93 created this revision.
njames93 added reviewers: aaron.ballman, alexfh, JonasToth, hokein.
njames93 added projects: clang-tools-extra, clang.
Herald added a subscriber: xazax.hun.
Clang-tidy has had an on growing issue with blindly adding all checks from a
module using the `-checks=somemodule-* ` option. If somemodule alias' another
enabled check. the check will get ran twice giving duplicated warning and fix
options. This can lead to fixes overlapping preventing them from being applied
or worse still applying twice resulting in malformed code. There are other
issues with ConfigOptions being different in the 2 different instances so one
instance of the check may be running a different set of options to the other.
This patch solves these issue by making clang-tidy only run 1 instance of an
aliased check even if it has been turned on with multiple names. Warning
messages are appended with the original check name if it has been enabled(via
cmd line, or config file). If the original check hasn't been enabled then the
name will be the first enabled alias that was registered.
Config options are also handled in a similar fashion, see example
[{ key: MyAliasedName.Option1, value: 0 }.
{ key: OriginalCheckName.Option1, value: 1 }]
If the check `original-check-name` was turned on, then `Option1` will be parsed
as being `1`, otherwise it ignores `OriginalCheckName.Option1` key and falls
back to the first enabled alias check that declares `Option1`
To declare a check as an alias when you register then check, just add the check
this alias' to as a second parameter
CheckFactories.registerCheck<readability::BracesAroundStatementsCheck>(
"hicpp-braces-around-statements");
// Update to
CheckFactories.registerCheck<readability::BracesAroundStatementsCheck>(
"hicpp-braces-around-statements", "readability-braces-around-statements");
I haven't added in all the alias declarations in just yet (If you want me to
just ask), nor have I added test cases. However once all the alias declarations
are added in, the tests fall nicely in line with the current test
infrastructure.
Adding this line to the readability-braces-around-statements checks causes no
errors, no multiple fix attempts or warning messages.
// RUN: %check_clang_tidy %s
readability-braces-around-statements,hicpp-readability-braces-around-statements
%t
Likewise changing it to
// RUN: %check_clang_tidy %s hicpp-readability-braces-around-statements %t
Also runs without a hitch (obviously warning messages are appended with
`[hicpp-readability-braces-around-statements]` instead of
`[readability-braces-around-statements]` as above.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D72566
Files:
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
clang-tools-extra/clang-tidy/ClangTidyCheck.h
clang-tools-extra/clang-tidy/ClangTidyModule.cpp
clang-tools-extra/clang-tidy/ClangTidyModule.h
clang-tools-extra/docs/ReleaseNotes.rst
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,6 +67,9 @@
Improvements to clang-tidy
--------------------------
+- Clang tidy is now aware of alias checks and will only run one instance of
+ an alias check that's enabled by different names.
+
New checks
^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/ClangTidyModule.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyModule.h
+++ clang-tools-extra/clang-tidy/ClangTidyModule.h
@@ -11,6 +11,7 @@
#include "ClangTidy.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
#include <functional>
#include <map>
#include <memory>
@@ -34,6 +35,13 @@
/// For all checks that have default constructors, use \c registerCheck.
void registerCheckFactory(StringRef Name, CheckFactory Factory);
+ /// Registers check \p Factory with name \p Name that is an alias to
+ /// another check called \p AliasToName.
+ ///
+ /// For all checks that have default constructors, use \c registerCheck.
+ void registerCheckFactory(StringRef Name, StringRef AliasToName,
+ CheckFactory Factory);
+
/// Registers the \c CheckType with the name \p Name.
///
/// This method should be used for all \c ClangTidyChecks that don't require
@@ -62,6 +70,16 @@
});
}
+ /// Registers the \c CheckType with the name \p Name that is an alias to
+ /// another check called \p AliasToName.
+ template <typename CheckType>
+ void registerCheck(StringRef CheckName, StringRef AliasToName) {
+ registerCheckFactory(CheckName, AliasToName,
+ [](StringRef Name, ClangTidyContext *Context) {
+ return std::make_unique<CheckType>(Name, Context);
+ });
+ }
+
/// Create instances of checks that are enabled.
std::vector<std::unique_ptr<ClangTidyCheck>>
createChecks(ClangTidyContext *Context);
@@ -69,10 +87,22 @@
typedef std::map<std::string, CheckFactory> FactoryMap;
FactoryMap::const_iterator begin() const { return Factories.begin(); }
FactoryMap::const_iterator end() const { return Factories.end(); }
- bool empty() const { return Factories.empty(); }
+ typedef std::map<std::string, std::pair<std::string, CheckFactory>>
+ AliasFactoryMap;
+ AliasFactoryMap::const_iterator alias_begin() const {
+ return AliasFactories.begin();
+ }
+ AliasFactoryMap::const_iterator alias_end() const {
+ return AliasFactories.end();
+ }
+ llvm::iterator_range<AliasFactoryMap::const_iterator> aliases() const {
+ return {alias_begin(), alias_end()};
+ }
+ bool empty() const { return Factories.empty() && AliasFactories.empty(); }
private:
FactoryMap Factories;
+ AliasFactoryMap AliasFactories;
};
/// A clang-tidy module groups a number of \c ClangTidyChecks and gives
Index: clang-tools-extra/clang-tidy/ClangTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyModule.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyModule.cpp
@@ -11,15 +11,31 @@
//===----------------------------------------------------------------------===//
#include "ClangTidyModule.h"
+#include "llvm/Support/raw_ostream.h"
+#include <utility>
namespace clang {
namespace tidy {
void ClangTidyCheckFactories::registerCheckFactory(StringRef Name,
CheckFactory Factory) {
+ if (AliasFactories.count(Name))
+ return; // Check already registered as alias
Factories[Name] = std::move(Factory);
}
+void ClangTidyCheckFactories::registerCheckFactory(StringRef Name,
+ StringRef AliasToName,
+ CheckFactory Factory) {
+ Factories.erase(AliasToName);
+ auto &Current = AliasFactories[AliasToName];
+ if (Current.second) // Check if already declared
+ Current.first.append((";" + Name).str());
+ else
+ Current = std::make_pair(Name, std::move(Factory));
+ assert(AliasFactories.count(AliasToName) != 0);
+}
+
std::vector<std::unique_ptr<ClangTidyCheck>>
ClangTidyCheckFactories::createChecks(ClangTidyContext *Context) {
std::vector<std::unique_ptr<ClangTidyCheck>> Checks;
@@ -27,6 +43,23 @@
if (Context->isCheckEnabled(Factory.first))
Checks.emplace_back(Factory.second(Factory.first, Context));
}
+ for (const auto &AliasFactory : AliasFactories) {
+ SmallString<256> Buffer;
+ llvm::raw_svector_ostream Builder(Buffer);
+ if (Context->isCheckEnabled(AliasFactory.first))
+ Builder << AliasFactory.first << ';';
+ SmallVector<StringRef, 4> Split;
+ StringRef AliasNames = AliasFactory.second.first;
+ AliasNames.split(Split, ';', -1, false);
+ for (StringRef Aliases : Split) {
+ if (Context->isCheckEnabled(Aliases))
+ Builder << Aliases << ';';
+ }
+ if (!Buffer.empty()) {
+ Buffer.pop_back(); // Remove the last ';'
+ Checks.emplace_back(AliasFactory.second.second(Buffer, Context));
+ }
+ }
return Checks;
}
Index: clang-tools-extra/clang-tidy/ClangTidyCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/ClangTidyCheck.h
@@ -170,7 +170,7 @@
int64_t Value) const;
private:
- std::string NamePrefix;
+ SmallVector<std::string, 2> NamePrefixes;
const ClangTidyOptions::OptionMap &CheckOptions;
};
Index: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
@@ -7,12 +7,13 @@
//===----------------------------------------------------------------------===//
#include "ClangTidyCheck.h"
+#include "llvm/ADT/SmallVector.h"
namespace clang {
namespace tidy {
ClangTidyCheck::ClangTidyCheck(StringRef CheckName, ClangTidyContext *Context)
- : CheckName(CheckName), Context(Context),
+ : CheckName(CheckName.split(';').first), Context(Context),
Options(CheckName, Context->getOptions().CheckOptions) {
assert(Context != nullptr);
assert(!CheckName.empty());
@@ -30,35 +31,46 @@
check(Result);
}
-ClangTidyCheck::OptionsView::OptionsView(StringRef CheckName,
- const ClangTidyOptions::OptionMap &CheckOptions)
- : NamePrefix(CheckName.str() + "."), CheckOptions(CheckOptions) {}
+ClangTidyCheck::OptionsView::OptionsView(
+ StringRef CheckName, const ClangTidyOptions::OptionMap &CheckOptions)
+ : CheckOptions(CheckOptions) {
+ llvm::SmallVector<StringRef, 4> CheckNames;
+ CheckName.split(CheckNames, ';', -1, false);
+ for (const auto &Name : CheckNames) {
+ NamePrefixes.emplace_back((Name + ".").str());
+ }
+ assert(!NamePrefixes.empty());
+}
std::string ClangTidyCheck::OptionsView::get(StringRef LocalName,
StringRef Default) const {
- const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
- if (Iter != CheckOptions.end())
- return Iter->second;
+ for (const auto &NamePrefix : NamePrefixes) {
+ const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str());
+ if (Iter != CheckOptions.end())
+ return Iter->second;
+ }
return Default;
}
std::string
ClangTidyCheck::OptionsView::getLocalOrGlobal(StringRef LocalName,
StringRef Default) const {
- auto Iter = CheckOptions.find(NamePrefix + LocalName.str());
- if (Iter != CheckOptions.end())
- return Iter->second;
- // Fallback to global setting, if present.
- Iter = CheckOptions.find(LocalName.str());
- if (Iter != CheckOptions.end())
- return Iter->second;
+ for (const auto &NamePrefix : NamePrefixes) {
+ auto Iter = CheckOptions.find(NamePrefix + LocalName.str());
+ if (Iter != CheckOptions.end())
+ return Iter->second;
+ // Fallback to global setting, if present.
+ Iter = CheckOptions.find(LocalName.str());
+ if (Iter != CheckOptions.end())
+ return Iter->second;
+ }
return Default;
}
void ClangTidyCheck::OptionsView::store(ClangTidyOptions::OptionMap &Options,
StringRef LocalName,
StringRef Value) const {
- Options[NamePrefix + LocalName.str()] = Value;
+ Options[NamePrefixes.front() + LocalName.str()] = Value;
}
void ClangTidyCheck::OptionsView::store(ClangTidyOptions::OptionMap &Options,
Index: clang-tools-extra/clang-tidy/ClangTidy.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -445,6 +445,17 @@
if (Context.isCheckEnabled(CheckFactory.first))
CheckNames.push_back(CheckFactory.first);
}
+ for (const auto &AliasCheckFactory : CheckFactories->aliases()) {
+ if (Context.isCheckEnabled(AliasCheckFactory.first))
+ CheckNames.push_back(AliasCheckFactory.first);
+ for (auto StringRefPair =
+ StringRef(AliasCheckFactory.second.first).split(';');
+ !StringRefPair.first.empty();
+ StringRefPair = StringRefPair.second.split(';')) {
+ if (Context.isCheckEnabled(StringRefPair.first))
+ CheckNames.push_back(StringRefPair.first);
+ }
+ }
#if CLANG_ENABLE_STATIC_ANALYZER
for (const auto &AnalyzerCheck : getAnalyzerCheckersAndPackages(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
