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

[Justin Bogner]

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;
    }

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)) {}