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

Manuel Klimek klimek at google.com
Tue Oct 7 04:03:17 PDT 2014


================
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:
> > > > > > 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.

http://reviews.llvm.org/D5602






More information about the cfe-commits mailing list