From 82289aa6c88ad9840369db294cc66ed829e8c435 Mon Sep 17 00:00:00 2001 From: Nathan James Date: Mon, 1 Mar 2021 17:55:16 +0000 Subject: [PATCH] [clang-tidy] Remove OptionError The interface served a purpose, but since the ability to emit diagnostics when parsing configuration was added, its become mostly redundant. Emitting the diagnostic and removing the boilerplate is much cleaner. Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D97614 --- clang-tools-extra/clang-tidy/ClangTidyCheck.cpp | 151 +++++------ clang-tools-extra/clang-tidy/ClangTidyCheck.h | 288 ++++++--------------- .../readability/IdentifierNamingCheck.cpp | 2 +- .../unittests/clang-tidy/ClangTidyOptionsTest.cpp | 156 ++++++----- 4 files changed, 239 insertions(+), 358 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp index e1bea43..46b69ed 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp @@ -15,30 +15,6 @@ namespace clang { namespace tidy { -char MissingOptionError::ID; -char UnparseableEnumOptionError::ID; -char UnparseableIntegerOptionError::ID; - -std::string MissingOptionError::message() const { - llvm::SmallString<128> Buffer({"option not found '", OptionName, "'"}); - return std::string(Buffer); -} - -std::string UnparseableEnumOptionError::message() const { - llvm::SmallString<256> Buffer({"invalid configuration value '", LookupValue, - "' for option '", LookupName, "'"}); - if (SuggestedValue) - Buffer.append({"; did you mean '", *SuggestedValue, "'?"}); - return std::string(Buffer); -} - -std::string UnparseableIntegerOptionError::message() const { - llvm::SmallString<256> Buffer({"invalid configuration value '", LookupValue, - "' for option '", LookupName, "'; expected ", - (IsBoolean ? "a bool" : "an integer value")}); - return std::string(Buffer); -} - ClangTidyCheck::ClangTidyCheck(StringRef CheckName, ClangTidyContext *Context) : CheckName(CheckName), Context(Context), Options(CheckName, Context->getOptions().CheckOptions, Context) { @@ -75,12 +51,12 @@ ClangTidyCheck::OptionsView::OptionsView( : NamePrefix(CheckName.str() + "."), CheckOptions(CheckOptions), Context(Context) {} -llvm::Expected +llvm::Optional ClangTidyCheck::OptionsView::get(StringRef LocalName) const { const auto &Iter = CheckOptions.find(NamePrefix + LocalName.str()); if (Iter != CheckOptions.end()) return Iter->getValue().Value; - return llvm::make_error((NamePrefix + LocalName).str()); + return None; } static ClangTidyOptions::OptionMap::const_iterator @@ -97,16 +73,16 @@ findPriorityOption(const ClangTidyOptions::OptionMap &Options, StringRef NamePre return IterGlobal; } -llvm::Expected +llvm::Optional ClangTidyCheck::OptionsView::getLocalOrGlobal(StringRef LocalName) const { auto Iter = findPriorityOption(CheckOptions, NamePrefix, LocalName); if (Iter != CheckOptions.end()) return Iter->getValue().Value; - return llvm::make_error((NamePrefix + LocalName).str()); + return None; } -static llvm::Expected getAsBool(StringRef Value, - const llvm::Twine &LookupName) { +static Optional getAsBool(StringRef Value, + const llvm::Twine &LookupName) { if (llvm::Optional Parsed = llvm::yaml::parseBool(Value)) return *Parsed; @@ -115,46 +91,30 @@ static llvm::Expected getAsBool(StringRef Value, long long Number; if (!Value.getAsInteger(10, Number)) return Number != 0; - return llvm::make_error(LookupName.str(), - Value.str(), true); + return None; } template <> -llvm::Expected +llvm::Optional ClangTidyCheck::OptionsView::get(StringRef LocalName) const { - llvm::Expected ValueOr = get(LocalName); - if (ValueOr) - return getAsBool(*ValueOr, NamePrefix + LocalName); - return ValueOr.takeError(); -} - -template <> -bool ClangTidyCheck::OptionsView::get(StringRef LocalName, - bool Default) const { - llvm::Expected ValueOr = get(LocalName); - if (ValueOr) - return *ValueOr; - reportOptionParsingError(ValueOr.takeError()); - return Default; + if (llvm::Optional ValueOr = get(LocalName)) { + if (auto Result = getAsBool(*ValueOr, NamePrefix + LocalName)) + return Result; + diagnoseBadBooleanOption(NamePrefix + LocalName, *ValueOr); + } + return None; } template <> -llvm::Expected +llvm::Optional ClangTidyCheck::OptionsView::getLocalOrGlobal(StringRef LocalName) const { auto Iter = findPriorityOption(CheckOptions, NamePrefix, LocalName); - if (Iter != CheckOptions.end()) - return getAsBool(Iter->getValue().Value, Iter->getKey()); - return llvm::make_error((NamePrefix + LocalName).str()); -} - -template <> -bool ClangTidyCheck::OptionsView::getLocalOrGlobal(StringRef LocalName, - bool Default) const { - llvm::Expected ValueOr = getLocalOrGlobal(LocalName); - if (ValueOr) - return *ValueOr; - reportOptionParsingError(ValueOr.takeError()); - return Default; + if (Iter != CheckOptions.end()) { + if (auto Result = getAsBool(Iter->getValue().Value, Iter->getKey())) + return Result; + diagnoseBadBooleanOption(Iter->getKey(), Iter->getValue().Value); + } + return None; } void ClangTidyCheck::OptionsView::store(ClangTidyOptions::OptionMap &Options, @@ -176,14 +136,14 @@ void ClangTidyCheck::OptionsView::store( store(Options, LocalName, Value ? StringRef("true") : StringRef("false")); } -llvm::Expected ClangTidyCheck::OptionsView::getEnumInt( +llvm::Optional ClangTidyCheck::OptionsView::getEnumInt( StringRef LocalName, ArrayRef Mapping, bool CheckGlobal, bool IgnoreCase) const { auto Iter = CheckGlobal ? findPriorityOption(CheckOptions, NamePrefix, LocalName) : CheckOptions.find((NamePrefix + LocalName).str()); if (Iter == CheckOptions.end()) - return llvm::make_error((NamePrefix + LocalName).str()); + return None; StringRef Value = Iter->getValue().Value; StringRef Closest; @@ -206,39 +166,54 @@ llvm::Expected ClangTidyCheck::OptionsView::getEnumInt( } } if (EditDistance < 3) - return llvm::make_error( - Iter->getKey().str(), Iter->getValue().Value, Closest.str()); - return llvm::make_error(Iter->getKey().str(), - Iter->getValue().Value); + diagnoseBadEnumOption(Iter->getKey().str(), Iter->getValue().Value, + Closest); + else + diagnoseBadEnumOption(Iter->getKey().str(), Iter->getValue().Value); + return None; } -void ClangTidyCheck::OptionsView::reportOptionParsingError( - llvm::Error &&Err) const { - if (auto RemainingErrors = - llvm::handleErrors(std::move(Err), [](const MissingOptionError &) {})) - Context->configurationDiag(llvm::toString(std::move(RemainingErrors))); +static constexpr llvm::StringLiteral ConfigWarning( + "invalid configuration value '%0' for option '%1'%select{|; expected a " + "bool|; expected an integer|; did you mean '%3'?}2"); + +void ClangTidyCheck::OptionsView::diagnoseBadBooleanOption( + const Twine &Lookup, StringRef Unparsed) const { + SmallString<64> Buffer; + Context->configurationDiag(ConfigWarning) + << Unparsed << Lookup.toStringRef(Buffer) << 1; } -template <> -Optional ClangTidyCheck::OptionsView::getOptional( - StringRef LocalName) const { - if (auto ValueOr = get(LocalName)) - return *ValueOr; - else - consumeError(ValueOr.takeError()); - return llvm::None; +void ClangTidyCheck::OptionsView::diagnoseBadIntegerOption( + const Twine &Lookup, StringRef Unparsed) const { + SmallString<64> Buffer; + Context->configurationDiag(ConfigWarning) + << Unparsed << Lookup.toStringRef(Buffer) << 2; } -template <> -Optional -ClangTidyCheck::OptionsView::getOptionalLocalOrGlobal( - StringRef LocalName) const { - if (auto ValueOr = getLocalOrGlobal(LocalName)) - return *ValueOr; +void ClangTidyCheck::OptionsView::diagnoseBadEnumOption( + const Twine &Lookup, StringRef Unparsed, StringRef Suggestion) const { + SmallString<64> Buffer; + auto Diag = Context->configurationDiag(ConfigWarning) + << Unparsed << Lookup.toStringRef(Buffer); + if (Suggestion.empty()) + Diag << 0; else - consumeError(ValueOr.takeError()); - return llvm::None; + Diag << 3 << Suggestion; } +std::string ClangTidyCheck::OptionsView::get(StringRef LocalName, + StringRef Default) const { + if (llvm::Optional Val = get(LocalName)) + return std::move(*Val); + return Default.str(); +} +std::string +ClangTidyCheck::OptionsView::getLocalOrGlobal(StringRef LocalName, + StringRef Default) const { + if (llvm::Optional Val = getLocalOrGlobal(LocalName)) + return std::move(*Val); + return Default.str(); +} } // namespace tidy } // namespace clang diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.h b/clang-tools-extra/clang-tidy/ClangTidyCheck.h index 9fa0d63..20e9b8e 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyCheck.h +++ b/clang-tools-extra/clang-tidy/ClangTidyCheck.h @@ -14,7 +14,6 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Basic/Diagnostic.h" #include "llvm/ADT/Optional.h" -#include "llvm/Support/Error.h" #include #include #include @@ -33,65 +32,6 @@ template struct OptionEnumMapping { static ArrayRef> getEnumMapping() = delete; }; -template class OptionError : public llvm::ErrorInfo { - std::error_code convertToErrorCode() const override { - return llvm::inconvertibleErrorCode(); - } - -public: - void log(raw_ostream &OS) const override { OS << this->message(); } -}; - -class MissingOptionError : public OptionError { -public: - explicit MissingOptionError(std::string OptionName) - : OptionName(OptionName) {} - - std::string message() const override; - static char ID; -private: - const std::string OptionName; -}; - -class UnparseableEnumOptionError - : public OptionError { -public: - explicit UnparseableEnumOptionError(std::string LookupName, - std::string LookupValue) - : LookupName(LookupName), LookupValue(LookupValue) {} - explicit UnparseableEnumOptionError(std::string LookupName, - std::string LookupValue, - std::string SuggestedValue) - : LookupName(LookupName), LookupValue(LookupValue), - SuggestedValue(SuggestedValue) {} - - std::string message() const override; - static char ID; - -private: - const std::string LookupName; - const std::string LookupValue; - const llvm::Optional SuggestedValue; -}; - -class UnparseableIntegerOptionError - : public OptionError { -public: - explicit UnparseableIntegerOptionError(std::string LookupName, - std::string LookupValue, - bool IsBoolean = false) - : LookupName(LookupName), LookupValue(LookupValue), IsBoolean(IsBoolean) { - } - - std::string message() const override; - static char ID; - -private: - const std::string LookupName; - const std::string LookupValue; - const bool IsBoolean; -}; - /// Base class for all clang-tidy checks. /// /// To implement a ``ClangTidyCheck``, write a subclass and override some of the @@ -198,6 +138,13 @@ public: /// Methods of this class prepend ``CheckName + "."`` to translate check-local /// option names to global option names. class OptionsView { + void diagnoseBadIntegerOption(const Twine &Lookup, + StringRef Unparsed) const; + void diagnoseBadBooleanOption(const Twine &Lookup, + StringRef Unparsed) const; + void diagnoseBadEnumOption(const Twine &Lookup, StringRef Unparsed, + StringRef Suggestion = StringRef()) const; + public: /// Initializes the instance using \p CheckName + "." as a prefix. OptionsView(StringRef CheckName, @@ -207,30 +154,24 @@ public: /// Read a named option from the ``Context``. /// /// Reads the option with the check-local name \p LocalName from the - /// ``CheckOptions``. If the corresponding key is not present, returns - /// a ``MissingOptionError``. - llvm::Expected get(StringRef LocalName) const; + /// ``CheckOptions``. If the corresponding key is not present, return + /// ``None``. + llvm::Optional get(StringRef LocalName) const; /// Read a named option from the ``Context``. /// /// Reads the option with the check-local name \p LocalName from the /// ``CheckOptions``. If the corresponding key is not present, returns /// \p Default. - std::string get(StringRef LocalName, StringRef Default) const { - if (llvm::Expected Val = get(LocalName)) - return *Val; - else - llvm::consumeError(Val.takeError()); - return Default.str(); - } + std::string get(StringRef LocalName, StringRef Default) const; /// Read a named option from the ``Context``. /// /// Reads the option with the check-local name \p LocalName from local or /// global ``CheckOptions``. Gets local option first. If local is not /// present, falls back to get global option. If global option is not - /// present either, returns a ``MissingOptionError``. - llvm::Expected getLocalOrGlobal(StringRef LocalName) const; + /// present either, return ``None``. + llvm::Optional getLocalOrGlobal(StringRef LocalName) const; /// Read a named option from the ``Context``. /// @@ -238,48 +179,42 @@ public: /// global ``CheckOptions``. Gets local option first. If local is not /// present, falls back to get global option. If global option is not /// present either, returns \p Default. - std::string getLocalOrGlobal(StringRef LocalName, StringRef Default) const { - if (llvm::Expected Val = getLocalOrGlobal(LocalName)) - return *Val; - else - llvm::consumeError(Val.takeError()); - return Default.str(); - } + std::string getLocalOrGlobal(StringRef LocalName, StringRef Default) const; /// Read a named option from the ``Context`` and parse it as an /// integral type ``T``. /// /// Reads the option with the check-local name \p LocalName from the - /// ``CheckOptions``. If the corresponding key is not present, returns - /// a ``MissingOptionError``. If the corresponding key can't be parsed as - /// a ``T``, return an ``UnparseableIntegerOptionError``. + /// ``CheckOptions``. If the corresponding key is not present, return + /// ``None``. + /// + /// If the corresponding key can't be parsed as a ``T``, emit a + /// diagnostic and return ``None``. template - std::enable_if_t::value, llvm::Expected> + std::enable_if_t::value, llvm::Optional> get(StringRef LocalName) const { - if (llvm::Expected Value = get(LocalName)) { + if (llvm::Optional Value = get(LocalName)) { T Result{}; if (!StringRef(*Value).getAsInteger(10, Result)) return Result; - return llvm::make_error( - (NamePrefix + LocalName).str(), *Value); - } else - return std::move(Value.takeError()); + diagnoseBadIntegerOption(NamePrefix + LocalName, *Value); + } + return None; } /// Read a named option from the ``Context`` and parse it as an /// integral type ``T``. /// /// Reads the option with the check-local name \p LocalName from the - /// ``CheckOptions``. If the corresponding key is not present or it can't be - /// parsed as a ``T``, returns \p Default. + /// ``CheckOptions``. If the corresponding key is not present, return + /// \p Default. + /// + /// If the corresponding key can't be parsed as a ``T``, emit a + /// diagnostic and return \p Default. template std::enable_if_t::value, T> get(StringRef LocalName, T Default) const { - if (llvm::Expected ValueOr = get(LocalName)) - return *ValueOr; - else - reportOptionParsingError(ValueOr.takeError()); - return Default; + return get(LocalName).getValueOr(Default); } /// Read a named option from the ``Context`` and parse it as an @@ -288,27 +223,27 @@ public: /// Reads the option with the check-local name \p LocalName from local or /// global ``CheckOptions``. Gets local option first. If local is not /// present, falls back to get global option. If global option is not - /// present either, returns a ``MissingOptionError``. If the corresponding - /// key can't be parsed as a ``T``, return an - /// ``UnparseableIntegerOptionError``. + /// present either, return ``None``. + /// + /// If the corresponding key can't be parsed as a ``T``, emit a + /// diagnostic and return ``None``. template - std::enable_if_t::value, llvm::Expected> + std::enable_if_t::value, llvm::Optional> getLocalOrGlobal(StringRef LocalName) const { - llvm::Expected ValueOr = get(LocalName); + llvm::Optional ValueOr = get(LocalName); bool IsGlobal = false; if (!ValueOr) { IsGlobal = true; - llvm::consumeError(ValueOr.takeError()); ValueOr = getLocalOrGlobal(LocalName); if (!ValueOr) - return std::move(ValueOr.takeError()); + return None; } T Result{}; if (!StringRef(*ValueOr).getAsInteger(10, Result)) return Result; - return llvm::make_error( - (IsGlobal ? LocalName.str() : (NamePrefix + LocalName).str()), - *ValueOr); + diagnoseBadIntegerOption( + IsGlobal ? Twine(LocalName) : NamePrefix + LocalName, *ValueOr); + return None; } /// Read a named option from the ``Context`` and parse it as an @@ -317,54 +252,53 @@ public: /// Reads the option with the check-local name \p LocalName from local or /// global ``CheckOptions``. Gets local option first. If local is not /// present, falls back to get global option. If global option is not - /// present either or it can't be parsed as a ``T``, returns \p Default. + /// present either, return \p Default. + /// + /// If the corresponding key can't be parsed as a ``T``, emit a + /// diagnostic and return \p Default. template std::enable_if_t::value, T> getLocalOrGlobal(StringRef LocalName, T Default) const { - if (llvm::Expected ValueOr = getLocalOrGlobal(LocalName)) - return *ValueOr; - else - reportOptionParsingError(ValueOr.takeError()); - return Default; + return getLocalOrGlobal(LocalName).getValueOr(Default); } /// Read a named option from the ``Context`` and parse it as an /// enum type ``T``. /// /// Reads the option with the check-local name \p LocalName from the - /// ``CheckOptions``. If the corresponding key is not present, returns a - /// ``MissingOptionError``. If the key can't be parsed as a ``T`` returns a - /// ``UnparseableEnumOptionError``. + /// ``CheckOptions``. If the corresponding key is not present, return + /// ``None``. + /// + /// If the corresponding key can't be parsed as a ``T``, emit a + /// diagnostic and return ``None``. /// /// \ref clang::tidy::OptionEnumMapping must be specialized for ``T`` to /// supply the mapping required to convert between ``T`` and a string. template - std::enable_if_t::value, llvm::Expected> + std::enable_if_t::value, llvm::Optional> get(StringRef LocalName, bool IgnoreCase = false) const { - if (llvm::Expected ValueOr = + if (llvm::Optional ValueOr = getEnumInt(LocalName, typeEraseMapping(), false, IgnoreCase)) return static_cast(*ValueOr); - else - return std::move(ValueOr.takeError()); + return None; } /// Read a named option from the ``Context`` and parse it as an /// enum type ``T``. /// /// Reads the option with the check-local name \p LocalName from the - /// ``CheckOptions``. If the corresponding key is not present or it can't be - /// parsed as a ``T``, returns \p Default. + /// ``CheckOptions``. If the corresponding key is not present, return + /// \p Default. + /// + /// If the corresponding key can't be parsed as a ``T``, emit a + /// diagnostic and return \p Default. /// /// \ref clang::tidy::OptionEnumMapping must be specialized for ``T`` to /// supply the mapping required to convert between ``T`` and a string. template std::enable_if_t::value, T> get(StringRef LocalName, T Default, bool IgnoreCase = false) const { - if (auto ValueOr = get(LocalName, IgnoreCase)) - return *ValueOr; - else - reportOptionParsingError(ValueOr.takeError()); - return Default; + return get(LocalName, IgnoreCase).getValueOr(Default); } /// Read a named option from the ``Context`` and parse it as an @@ -373,19 +307,20 @@ public: /// Reads the option with the check-local name \p LocalName from local or /// global ``CheckOptions``. Gets local option first. If local is not /// present, falls back to get global option. If global option is not - /// present either, returns a ``MissingOptionError``. If the key can't be - /// parsed as a ``T`` returns a ``UnparseableEnumOptionError``. + /// present either, returns ``None``. + /// + /// If the corresponding key can't be parsed as a ``T``, emit a + /// diagnostic and return ``None``. /// /// \ref clang::tidy::OptionEnumMapping must be specialized for ``T`` to /// supply the mapping required to convert between ``T`` and a string. template - std::enable_if_t::value, llvm::Expected> + std::enable_if_t::value, llvm::Optional> getLocalOrGlobal(StringRef LocalName, bool IgnoreCase = false) const { - if (llvm::Expected ValueOr = + if (llvm::Optional ValueOr = getEnumInt(LocalName, typeEraseMapping(), true, IgnoreCase)) return static_cast(*ValueOr); - else - return std::move(ValueOr.takeError()); + return None; } /// Read a named option from the ``Context`` and parse it as an @@ -394,7 +329,10 @@ public: /// Reads the option with the check-local name \p LocalName from local or /// global ``CheckOptions``. Gets local option first. If local is not /// present, falls back to get global option. If global option is not - /// present either or it can't be parsed as a ``T``, returns \p Default. + /// present either return \p Default. + /// + /// If the corresponding key can't be parsed as a ``T``, emit a + /// diagnostic and return \p Default. /// /// \ref clang::tidy::OptionEnumMapping must be specialized for ``T`` to /// supply the mapping required to convert between ``T`` and a string. @@ -402,36 +340,7 @@ public: std::enable_if_t::value, T> getLocalOrGlobal(StringRef LocalName, T Default, bool IgnoreCase = false) const { - if (auto ValueOr = getLocalOrGlobal(LocalName, IgnoreCase)) - return *ValueOr; - else - reportOptionParsingError(ValueOr.takeError()); - return Default; - } - - /// Returns the value for the option \p LocalName represented as a ``T``. - /// If the option is missing returns None, if the option can't be parsed - /// as a ``T``, log that to stderr and return None. - template - llvm::Optional getOptional(StringRef LocalName) const { - if (auto ValueOr = get(LocalName)) - return *ValueOr; - else - reportOptionParsingError(ValueOr.takeError()); - return llvm::None; - } - - /// Returns the value for the local or global option \p LocalName - /// represented as a ``T``. - /// If the option is missing returns None, if the - /// option can't be parsed as a ``T``, log that to stderr and return None. - template - llvm::Optional getOptionalLocalOrGlobal(StringRef LocalName) const { - if (auto ValueOr = getLocalOrGlobal(LocalName)) - return *ValueOr; - else - reportOptionParsingError(ValueOr.takeError()); - return llvm::None; + return getLocalOrGlobal(LocalName, IgnoreCase).getValueOr(Default); } /// Stores an option with the check-local name \p LocalName with @@ -470,7 +379,7 @@ public: private: using NameAndValue = std::pair; - llvm::Expected getEnumInt(StringRef LocalName, + llvm::Optional getEnumInt(StringRef LocalName, ArrayRef Mapping, bool CheckGlobal, bool IgnoreCase) const; @@ -491,8 +400,6 @@ public: void storeInt(ClangTidyOptions::OptionMap &Options, StringRef LocalName, int64_t Value) const; - /// Emits a diagnostic if \p Err is not a MissingOptionError. - void reportOptionParsingError(llvm::Error &&Err) const; std::string NamePrefix; const ClangTidyOptions::OptionMap &CheckOptions; @@ -516,44 +423,27 @@ protected: /// Read a named option from the ``Context`` and parse it as a bool. /// /// Reads the option with the check-local name \p LocalName from the -/// ``CheckOptions``. If the corresponding key is not present, returns -/// a ``MissingOptionError``. If the corresponding key can't be parsed as -/// a bool, return an ``UnparseableIntegerOptionError``. +/// ``CheckOptions``. If the corresponding key is not present, return +/// ``None``. +/// +/// If the corresponding key can't be parsed as a bool, emit a +/// diagnostic and return ``None``. template <> -llvm::Expected +llvm::Optional ClangTidyCheck::OptionsView::get(StringRef LocalName) const; /// Read a named option from the ``Context`` and parse it as a bool. /// /// Reads the option with the check-local name \p LocalName from the -/// ``CheckOptions``. If the corresponding key is not present or it can't be -/// parsed as a bool, returns \p Default. -template <> -bool ClangTidyCheck::OptionsView::get(StringRef LocalName, - bool Default) const; - -/// Read a named option from the ``Context`` and parse it as a bool. +/// ``CheckOptions``. If the corresponding key is not present, return +/// \p Default. /// -/// Reads the option with the check-local name \p LocalName from local or -/// global ``CheckOptions``. Gets local option first. If local is not -/// present, falls back to get global option. If global option is not -/// present either, returns a ``MissingOptionError``. If the corresponding -/// key can't be parsed as a bool, return an -/// ``UnparseableIntegerOptionError``. +/// If the corresponding key can't be parsed as a bool, emit a +/// diagnostic and return \p Default. template <> -llvm::Expected +llvm::Optional ClangTidyCheck::OptionsView::getLocalOrGlobal(StringRef LocalName) const; -/// Read a named option from the ``Context`` and parse it as a bool. -/// -/// Reads the option with the check-local name \p LocalName from local or -/// global ``CheckOptions``. Gets local option first. If local is not -/// present, falls back to get global option. If global option is not -/// present either or it can't be parsed as a bool, returns \p Default. -template <> -bool ClangTidyCheck::OptionsView::getLocalOrGlobal(StringRef LocalName, - bool Default) const; - /// Stores an option with the check-local name \p LocalName with /// bool value \p Value to \p Options. template <> @@ -561,18 +451,6 @@ void ClangTidyCheck::OptionsView::store( ClangTidyOptions::OptionMap &Options, StringRef LocalName, bool Value) const; -/// Returns the value for the option \p LocalName. -/// If the option is missing returns None. -template <> -Optional ClangTidyCheck::OptionsView::getOptional( - StringRef LocalName) const; - -/// Returns the value for the local or global option \p LocalName. -/// If the option is missing returns None. -template <> -Optional -ClangTidyCheck::OptionsView::getOptionalLocalOrGlobal( - StringRef LocalName) const; } // namespace tidy } // namespace clang diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp index fccff50..09b3cc2 100644 --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp @@ -157,7 +157,7 @@ getFileStyleFromOptions(const ClangTidyCheck::OptionsView &Options) { StyleString.pop_back(); StyleString.pop_back(); auto CaseOptional = - Options.getOptional(StyleString); + Options.get(StyleString); if (CaseOptional || !Prefix.empty() || !Postfix.empty() || !IgnoredRegexpStr.empty()) diff --git a/clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp b/clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp index 6447f34..c4fb8bd 100644 --- a/clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp +++ b/clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp @@ -190,6 +190,7 @@ MATCHER_P(DiagRange, P, "") { return arg.Range && *arg.Range == P; } using ::testing::AllOf; using ::testing::ElementsAre; +using ::testing::UnorderedElementsAre; TEST(ParseConfiguration, CollectDiags) { DiagCollecter Collector; @@ -243,7 +244,6 @@ public: return Options.getLocalOrGlobal(std::forward(Arguments)...); } }; -} // namespace #define CHECK_VAL(Value, Expected) \ do { \ @@ -252,17 +252,22 @@ public: EXPECT_EQ(*Item, Expected); \ } while (false) -#define CHECK_ERROR(Value, ErrorType, ExpectedMessage) \ - do { \ - auto Item = Value; \ - ASSERT_FALSE(Item); \ - ASSERT_TRUE(Item.errorIsA()); \ - ASSERT_FALSE(llvm::handleErrors( \ - Item.takeError(), [&](const ErrorType &Err) -> llvm::Error { \ - EXPECT_EQ(Err.message(), ExpectedMessage); \ - return llvm::Error::success(); \ - })); \ - } while (false) +MATCHER_P(ToolDiagMessage, M, "") { return arg.Message.Message == M; } +MATCHER_P(ToolDiagLevel, L, "") { return arg.DiagLevel == L; } + +} // namespace + +} // namespace test + +static constexpr auto Warning = tooling::Diagnostic::Warning; +static constexpr auto Error = tooling::Diagnostic::Error; + +static void PrintTo(const ClangTidyError &Err, ::std::ostream *OS) { + *OS << (Err.DiagLevel == Error ? "error: " : "warning: ") + << Err.Message.Message; +} + +namespace test { TEST(CheckOptionsValidation, MissingOptions) { ClangTidyOptions Options; @@ -273,21 +278,21 @@ TEST(CheckOptionsValidation, MissingOptions) { &DiagConsumer, false); Context.setDiagnosticsEngine(&DE); TestCheck TestCheck(&Context); - CHECK_ERROR(TestCheck.getLocal("Opt"), MissingOptionError, - "option not found 'test.Opt'"); + EXPECT_FALSE(TestCheck.getLocal("Opt").hasValue()); EXPECT_EQ(TestCheck.getLocal("Opt", "Unknown"), "Unknown"); + // Missing options aren't errors. + EXPECT_TRUE(DiagConsumer.take().empty()); } TEST(CheckOptionsValidation, ValidIntOptions) { ClangTidyOptions Options; auto &CheckOptions = Options.CheckOptions; - CheckOptions["test.IntExpected1"] = "1"; - CheckOptions["test.IntExpected2"] = "1WithMore"; - CheckOptions["test.IntExpected3"] = "NoInt"; - CheckOptions["GlobalIntExpected1"] = "1"; - CheckOptions["GlobalIntExpected2"] = "NoInt"; - CheckOptions["test.DefaultedIntInvalid"] = "NoInt"; + CheckOptions["test.IntExpected"] = "1"; + CheckOptions["test.IntInvalid1"] = "1WithMore"; + CheckOptions["test.IntInvalid2"] = "NoInt"; + CheckOptions["GlobalIntExpected"] = "1"; CheckOptions["GlobalIntInvalid"] = "NoInt"; + CheckOptions["test.DefaultedIntInvalid"] = "NoInt"; CheckOptions["test.BoolITrueValue"] = "1"; CheckOptions["test.BoolIFalseValue"] = "0"; CheckOptions["test.BoolTrueValue"] = "true"; @@ -304,22 +309,12 @@ TEST(CheckOptionsValidation, ValidIntOptions) { Context.setDiagnosticsEngine(&DE); TestCheck TestCheck(&Context); -#define CHECK_ERROR_INT(Name, Expected) \ - CHECK_ERROR(Name, UnparseableIntegerOptionError, Expected) - - CHECK_VAL(TestCheck.getIntLocal("IntExpected1"), 1); - CHECK_VAL(TestCheck.getIntGlobal("GlobalIntExpected1"), 1); - CHECK_ERROR_INT(TestCheck.getIntLocal("IntExpected2"), - "invalid configuration value '1WithMore' for option " - "'test.IntExpected2'; expected an integer value"); - CHECK_ERROR_INT(TestCheck.getIntLocal("IntExpected3"), - "invalid configuration value 'NoInt' for option " - "'test.IntExpected3'; expected an integer value"); - CHECK_ERROR_INT(TestCheck.getIntGlobal("GlobalIntExpected2"), - "invalid configuration value 'NoInt' for option " - "'GlobalIntExpected2'; expected an integer value"); + CHECK_VAL(TestCheck.getIntLocal("IntExpected"), 1); + CHECK_VAL(TestCheck.getIntGlobal("GlobalIntExpected"), 1); + EXPECT_FALSE(TestCheck.getIntLocal("IntInvalid1").hasValue()); + EXPECT_FALSE(TestCheck.getIntLocal("IntInvalid2").hasValue()); + EXPECT_FALSE(TestCheck.getIntGlobal("GlobalIntInvalid").hasValue()); ASSERT_EQ(TestCheck.getIntLocal("DefaultedIntInvalid", 1), 1); - ASSERT_EQ(TestCheck.getIntGlobal("GlobalIntInvalid", 1), 1); CHECK_VAL(TestCheck.getIntLocal("BoolITrueValue"), true); CHECK_VAL(TestCheck.getIntLocal("BoolIFalseValue"), false); @@ -327,11 +322,31 @@ TEST(CheckOptionsValidation, ValidIntOptions) { CHECK_VAL(TestCheck.getIntLocal("BoolFalseValue"), false); CHECK_VAL(TestCheck.getIntLocal("BoolTrueShort"), true); CHECK_VAL(TestCheck.getIntLocal("BoolFalseShort"), false); - CHECK_ERROR_INT(TestCheck.getIntLocal("BoolUnparseable"), - "invalid configuration value 'Nothing' for option " - "'test.BoolUnparseable'; expected a bool"); - -#undef CHECK_ERROR_INT + EXPECT_FALSE(TestCheck.getIntLocal("BoolUnparseable").hasValue()); + + EXPECT_THAT( + DiagConsumer.take(), + UnorderedElementsAre( + AllOf(ToolDiagMessage( + "invalid configuration value '1WithMore' for option " + "'test.IntInvalid1'; expected an integer"), + ToolDiagLevel(Warning)), + AllOf( + ToolDiagMessage("invalid configuration value 'NoInt' for option " + "'test.IntInvalid2'; expected an integer"), + ToolDiagLevel(Warning)), + AllOf( + ToolDiagMessage("invalid configuration value 'NoInt' for option " + "'GlobalIntInvalid'; expected an integer"), + ToolDiagLevel(Warning)), + AllOf(ToolDiagMessage( + "invalid configuration value 'NoInt' for option " + "'test.DefaultedIntInvalid'; expected an integer"), + ToolDiagLevel(Warning)), + AllOf(ToolDiagMessage( + "invalid configuration value 'Nothing' for option " + "'test.BoolUnparseable'; expected a bool"), + ToolDiagLevel(Warning)))); } TEST(ValidConfiguration, ValidEnumOptions) { @@ -350,11 +365,12 @@ TEST(ValidConfiguration, ValidEnumOptions) { ClangTidyContext Context(std::make_unique( ClangTidyGlobalOptions(), Options)); + ClangTidyDiagnosticConsumer DiagConsumer(Context); + DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions, + &DiagConsumer, false); + Context.setDiagnosticsEngine(&DE); TestCheck TestCheck(&Context); -#define CHECK_ERROR_ENUM(Name, Expected) \ - CHECK_ERROR(Name, UnparseableEnumOptionError, Expected) - CHECK_VAL(TestCheck.getIntLocal("Valid"), Colours::Red); CHECK_VAL(TestCheck.getIntGlobal("GlobalValid"), Colours::Violet); @@ -364,30 +380,42 @@ TEST(ValidConfiguration, ValidEnumOptions) { CHECK_VAL(TestCheck.getIntGlobal("GlobalValidWrongCase", /*IgnoreCase*/ true), Colours::Violet); - CHECK_ERROR_ENUM(TestCheck.getIntLocal("Invalid"), - "invalid configuration value " - "'Scarlet' for option 'test.Invalid'"); - CHECK_ERROR_ENUM(TestCheck.getIntLocal("ValidWrongCase"), - "invalid configuration value 'rED' for option " - "'test.ValidWrongCase'; did you mean 'Red'?"); - CHECK_ERROR_ENUM(TestCheck.getIntLocal("NearMiss"), - "invalid configuration value 'Oragne' for option " - "'test.NearMiss'; did you mean 'Orange'?"); - CHECK_ERROR_ENUM(TestCheck.getIntGlobal("GlobalInvalid"), - "invalid configuration value " - "'Purple' for option 'GlobalInvalid'"); - CHECK_ERROR_ENUM(TestCheck.getIntGlobal("GlobalValidWrongCase"), - "invalid configuration value 'vIOLET' for option " - "'GlobalValidWrongCase'; did you mean 'Violet'?"); - CHECK_ERROR_ENUM(TestCheck.getIntGlobal("GlobalNearMiss"), - "invalid configuration value 'Yelow' for option " - "'GlobalNearMiss'; did you mean 'Yellow'?"); - -#undef CHECK_ERROR_ENUM + + EXPECT_FALSE(TestCheck.getIntLocal("ValidWrongCase").hasValue()); + EXPECT_FALSE(TestCheck.getIntLocal("NearMiss").hasValue()); + EXPECT_FALSE(TestCheck.getIntGlobal("GlobalInvalid").hasValue()); + EXPECT_FALSE( + TestCheck.getIntGlobal("GlobalValidWrongCase").hasValue()); + EXPECT_FALSE(TestCheck.getIntGlobal("GlobalNearMiss").hasValue()); + + EXPECT_FALSE(TestCheck.getIntLocal("Invalid").hasValue()); + EXPECT_THAT( + DiagConsumer.take(), + UnorderedElementsAre( + AllOf(ToolDiagMessage("invalid configuration value " + "'Scarlet' for option 'test.Invalid'"), + ToolDiagLevel(Warning)), + AllOf(ToolDiagMessage("invalid configuration value 'rED' for option " + "'test.ValidWrongCase'; did you mean 'Red'?"), + ToolDiagLevel(Warning)), + AllOf( + ToolDiagMessage("invalid configuration value 'Oragne' for option " + "'test.NearMiss'; did you mean 'Orange'?"), + ToolDiagLevel(Warning)), + AllOf(ToolDiagMessage("invalid configuration value " + "'Purple' for option 'GlobalInvalid'"), + ToolDiagLevel(Warning)), + AllOf( + ToolDiagMessage("invalid configuration value 'vIOLET' for option " + "'GlobalValidWrongCase'; did you mean 'Violet'?"), + ToolDiagLevel(Warning)), + AllOf( + ToolDiagMessage("invalid configuration value 'Yelow' for option " + "'GlobalNearMiss'; did you mean 'Yellow'?"), + ToolDiagLevel(Warning)))); } #undef CHECK_VAL -#undef CHECK_ERROR } // namespace test } // namespace tidy -- 2.7.4