logan-5 added a comment.
Thanks for the patch. This seems like it'll make the check much more useful.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:48
+ Options.get("AllowedIdentifiers", ""))) {
+ for (const auto &Identifier : AllowedIdentifiers) {
+ if (!llvm::Regex(Identifier).isValid())
----------------
Nit: we should `AllowedIdentifiersRegex.reserve(AllowedIdentifiers.size())`
before the loop.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h:34
const std::vector<StringRef> AllowedIdentifiers;
+ llvm::SmallVector<llvm::Regex> AllowedIdentifiersRegex;
----------------
I understand that we need to keep the `AllowedIdentifiers` member around for
use in `storeOptions`. To avoid confusion, it might make sense to change its
name to be the "less default" name (something like `AllowedIdentifiersRaw`),
and give the vector of regexes the more intuitive name `AllowedIdentifiers`.
That would further avoid confusion since `getFailureInfoImpl` in the .cpp
currently refers to the vector of regexes using the name `AllowedIdentifiers`.
Also, this should probably be `const` for consistency. The loop in the
constructor body should arguably be moved into a helper function anyway, so
this member can be initialized in the constructor initializer list.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152764/new/
https://reviews.llvm.org/D152764
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
