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

[Manuel Klimek]

================
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:
> > 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?
> ScalarTraits::input returns a StringRef describing the error. We can also use StringRef to avoid impedance mismatch and make use of these strings in our error messages.
> 
> Or maybe ScalarTraits<> should be changed to use std::error_code with custom errors. I'll ask the authors' opinion.
I'd vote for renaming.

================
Comment at: clang-tidy/ClangTidy.h:68-80
@@ -67,1 +67,15 @@
 
+  /// \brief Read a named option from the \c Context and parse it as bool.
+  ///
+  /// Reads the option with the check-local name \p LocalName from the
+  /// \c CheckOptions. If the corresponding key is not present, returns
+  /// \p Default.
+  bool get(StringRef LocalName, bool Default) const {
+    std::string Value = get(LocalName, "");
+    if (Value == "true")
+      return true;
+    if (Value == "false")
+      return false;
+    return Default;
+  }
+
----------------
alexfh wrote:
> klimek wrote:
> > alexfh wrote:
> > > klimek wrote:
> > > > alexfh wrote:
> > > > > klimek wrote:
> > > > > > alexfh wrote:
> > > > > > > klimek wrote:
> > > > > > > > Why do we want unknown values to give Default? If we do, we should put it into the comments; can we rather make them given an error while parsing, and then do:
> > > > > > > > return get(LocalName, Default ? "true" : "false") == "true";
> > > > > > > We can't error out early enough as check options are parsed after the configuration is read. So I don't currently see a better way than to fall back to default.
> > > > > > Fall back to false?
> > > > > Could you explain why this would be better?
> > > > If we catch the error in the config later on, and give a nice error, it doesn't matter, and the code gets simpler.
> > > > 
> > > > If we cannot catch the error, I would like an explicitly given config to not lead to different behavior depending on the default value; I find that unexpected. Plus, I think it would make the code simpler.
> > > > If we catch the error in the config later on, and give a nice error,
> > > 
> > > Not sure yet if this can be done nicely.
> > > 
> > > > it doesn't matter, and the code gets simpler.
> > > 
> > > It doesn't make the code simpler, as we still need to handle missing value and return Default. 
> > > 
> > > > If we cannot catch the error, I would like an explicitly given config to not lead to different behavior depending on the default value; I find that unexpected. 
> > > 
> > > Why? On the opposite, I think that the case of invalid value is close to the case of missing value, so using the same default is more intuitive.
> > > 
> > > > Plus, I think it would make the code simpler.
> > > 
> > > Not really. The code is now common for all types having ScalarTraits<>. It also falls back to Default in case of error, and there I don't see another reasonable value to fall back to in case the parsing fails.
> > Ok, I'm convinced:
> > If this is really the same handling for all scalar values, I find it significantly worse if we silently eat up parsing errors. I'd prefer crashing with an error message if that's the only way possible.
> Crashing doesn't seem like a the best solution. We could (mis)use diagnostic output to report configuration errors. WDYT?
> 
> For now, I've changed OptionsView::get<> to propagate errors to the caller. And the caller can decide whether to crash in case of an error or handle it in a different way. I've changed two current users of this method to use ErrorOr<>::operator* which asserts that there's no value. Do we want to add some kind of a getValueOrCrash method to ErrorOr<> which would use report_fatal_error instead?
+1 to some form of diagnostic. Also +1 for handing to to the caller.
Is there a way where the caller can just ask whether there's a value?

http://reviews.llvm.org/D5602