[PATCH] [clang-tidy] Add support for boolean check options.

Marek Kurdej curdeius at gmail.com
Fri Oct 10 00:18:03 PDT 2014


================
Comment at: clang-tidy/ClangTidy.h:52-63
@@ -49,12 +51,14 @@
   /// \p Default.
-  std::string get(StringRef LocalName, std::string Default) const;
+  std::string get(StringRef LocalName, StringRef Default) const;
 
-  /// \brief Read a named option from the \c Context and parse it as an integral
-  /// type \c T.
+  /// \brief Read a named option from the \c Context and parse it as any
+  /// type \c T that has ScalarTraits<>.
   ///
   /// Reads the option with the check-local name \p LocalName from the
   /// \c CheckOptions. If the corresponding key is not present, returns
   /// \p Default.
   template <typename T>
-  typename std::enable_if<std::is_integral<T>::value, T>::type
+  typename std::enable_if<llvm::yaml::has_ScalarTraits<T>::value,
+                          llvm::ErrorOr<T>>::type
   get(StringRef LocalName, T Default) const {
+    std::string String = get(LocalName, "");
----------------
alexfh wrote:
> curdeius wrote:
> > Won't it be counterintuitive for the user of this code that `get(..., string)` returns a `string` and that `get(..., T)` returns `ErrorOr<T>`?
> The difference is that the string version doesn't do any parsing, so it can't encounter any errors. So maybe it should be named differently from the template version to avoid confusion.
Yes, I know that, but still it's a bit weird in my eyes to have functions with same name behaving as differently as this.
Renaming it is one valid approach.
Another one would be to add an argument indicating that something went wrong, e.g. `get(..., T Default, bool *Invalid = nullptr)`.
It would be up to the user to handle the error or not (on error, `get` will return Default).
In any case, we don't pass any more information with error_code than with a boolean...

What are your thoughts about it?

http://reviews.llvm.org/D5602






More information about the cfe-commits mailing list