================
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;
+ }
+
----------------
klimek wrote:
> 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?
We can add OptionsView::get overloads that return Optional<Something> (though
not sure why would callers bother using it instead of the versions with
default).
http://reviews.llvm.org/D5602
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
