[PATCH] Add ErrorOr<>::getOrDie() which uses report_fatal_error instead of assert.

[Alexander Kornienko]

On Fri, Oct 10, 2014 at 10:18 AM, Justin Bogner <mail at justinbogner.com>
wrote:

> Rafael Espíndola <rafael.espindola at gmail.com> writes:
> >>> I don't thing anyone disagrees that a nice error message is a feature,
> >>> but it might not be the priority of every code to use a SMDiagnostic.
> >>> Using report_fatal_error is a reasonable way to start (or an
> >>> improvement over a misplaced assert/unreachable, like we have in parts
> >>> of LLVM (pr20485 for example)).
> >>
> >> To be clear, I was actually just referring to the message that's being
> >> passed to report_fatal_error in the second point.
> >
> > Oh, I missed that point. It is probably better do to something like
> >
> > template<typename T>
> > const T& extractOrDie(const ErrorOr<T> &V, const char *Msg) {
> >   if (V)
> >     return *V;
> >   report_fatal_error(Msg);
> > }
>
> Sure, but I'm still not convinced this is worthwhile. The motivating
> example was apparently http://reviews.llvm.org/D5602, which looks to me
> like it wants to use this feature for something like this (**):
>
>     Foo::Foo(ErrorOr<Whatever> X)
>         : Whatever(extractOrDie(X, "the thing is broken") {}
>
> This can save a couple of lines of code, but I would argue it's quite a
> bit less clear than this:
>
>     Foo::Foo(ErrorOr<Whatever> X) {
>       if (X.hasError())
>         report_fatal_error("the thing is broken");
>       Whatever = *X;
>     }
>

Yes, I want to replace this kind of code with member initializers using
getOrDie/extractOrDie with or without an error message. The reason is to
reduce code duplication and keep the initialization in the initializer
list. And I thought that this abstraction may be useful elsewhere. If not,
I'll just end up putting extracOrDie in some clang-tidy header.


>
> Given that this kind of thing really should be rare, I'm not convinced
> that this is a valuable abstraction.
>
> (**) This is a bit simplified, since the ErrorOr appears to be a global
>      or something in D5602. I've based my assessment on the following
>      change, which I assume is the reason this review was referred to:
>
>      Index: clang-tidy/readability/NamespaceCommentCheck.cpp
>      ===================================================================
>      --- clang-tidy/readability/NamespaceCommentCheck.cpp
>      +++ clang-tidy/readability/NamespaceCommentCheck.cpp
>      @@ -25,9 +25,9 @@
>             NamespaceCommentPattern("^/[/*] *(end (of )?)?
> *(anonymous|unnamed)? *"
>                                     "namespace( +([a-zA-Z0-9_]+))?
> *(\\*/)?$",
>                                     llvm::Regex::IgnoreCase),
>      -      ShortNamespaceLines(Options.get("ShortNamespaceLines", 1u)),
>      -      SpacesBeforeComments(Options.get("SpacesBeforeComments",
>      -                                       Name.startswith("google") ?
> 2u : 1u)) {}
>      +      ShortNamespaceLines(*Options.get("ShortNamespaceLines", 1u)),
>      +      SpacesBeforeComments(*Options.get("SpacesBeforeComments",
>      +                                        Name.startswith("google") ?
> 2u : 1u)) {}
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141010/5c43ba0b/attachment.html>