aaron.ballman added a comment.
Thank you for working on this, I think it's going to be a very useful interface!
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:25
+ llvm::raw_svector_ostream Output(Buffer);
+ Output << "warning: Option not found '" << OptionName << "'\n";
+ return std::string(Buffer);
----------------
I think the diagnostic text should probably not start with a capital letter.
Also, the name of the classes are confusing in that they say error but the
diagnostic is a warning. When I hear "error", the class name makes me think
this would stop compilation and give a nonzero result code from the program.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:108-113
+ if (CheckGlobal && Iter == CheckOptions.end()) {
+ Iter = CheckOptions.find(LocalName.str());
+ }
+ if (Iter == CheckOptions.end()) {
+ return llvm::make_error<MissingOptionError>((NamePrefix +
LocalName).str());
+ }
----------------
Elide braces
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:122-124
+ } else if (Value.equals(NameAndEnum.first))
+ return NameAndEnum.second;
+ else if (Value.equals_lower(NameAndEnum.first)) {
----------------
Preference to add the braces to this case because the surrounding ones do as
well.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:41
+public:
+ MissingOptionError(std::string OptionName) : OptionName(OptionName) {}
+
----------------
Make the constructor `explicit`? (May want to consider the same for the other
ctors, but this one seems more dangerous.)
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:197
+ std::string get(StringRef LocalName, StringRef Default) const {
+ if (auto Val = get(LocalName))
+ return *Val;
----------------
Don't use `auto` as the type is not spelled out.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:201
+ llvm::consumeError(Val.takeError());
+ return std::string(Default);
+ }
----------------
`Default.str()` instead?
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:219-223
+ if (auto Val = getLocalOrGlobal(LocalName))
+ return *Val;
+ else
+ llvm::consumeError(Val.takeError());
+ return std::string(Default);
----------------
Same here as above.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.h:280
+ ValueOr = getLocalOrGlobal(LocalName);
+ if (!ValueOr) {
+ return std::move(ValueOr.takeError());
----------------
Elide braces
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77085/new/
https://reviews.llvm.org/D77085
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
