================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
