<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Oct 10, 2014 at 10:18 AM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> writes:<br>
>>> I don't thing anyone disagrees that a nice error message is a feature,<br>
>>> but it might not be the priority of every code to use a SMDiagnostic.<br>
>>> Using report_fatal_error is a reasonable way to start (or an<br>
>>> improvement over a misplaced assert/unreachable, like we have in parts<br>
>>> of LLVM (pr20485 for example)).<br>
>><br>
>> To be clear, I was actually just referring to the message that's being<br>
>> passed to report_fatal_error in the second point.<br>
><br>
> Oh, I missed that point. It is probably better do to something like<br>
><br>
> template<typename T><br>
> const T& extractOrDie(const ErrorOr<T> &V, const char *Msg) {<br>
>   if (V)<br>
>     return *V;<br>
>   report_fatal_error(Msg);<br>
> }<br>
<br>
</div></div>Sure, but I'm still not convinced this is worthwhile. The motivating<br>
example was apparently <a href="http://reviews.llvm.org/D5602" target="_blank">http://reviews.llvm.org/D5602</a>, which looks to me<br>
like it wants to use this feature for something like this (**):<br>
<br>
    Foo::Foo(ErrorOr<Whatever> X)<br>
        : Whatever(extractOrDie(X, "the thing is broken") {}<br>
<br>
This can save a couple of lines of code, but I would argue it's quite a<br>
bit less clear than this:<br>
<br>
    Foo::Foo(ErrorOr<Whatever> X) {<br>
      if (X.hasError())<br>
        report_fatal_error("the thing is broken");<br>
      Whatever = *X;<br>
    }<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Given that this kind of thing really should be rare, I'm not convinced<br>
that this is a valuable abstraction.<br>
<br>
(**) This is a bit simplified, since the ErrorOr appears to be a global<br>
     or something in D5602. I've based my assessment on the following<br>
     change, which I assume is the reason this review was referred to:<br>
<br>
     Index: clang-tidy/readability/NamespaceCommentCheck.cpp<br>
     ===================================================================<br>
     --- clang-tidy/readability/NamespaceCommentCheck.cpp<br>
     +++ clang-tidy/readability/NamespaceCommentCheck.cpp<br>
     @@ -25,9 +25,9 @@<br>
            NamespaceCommentPattern("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"<br>
                                    "namespace( +([a-zA-Z0-9_]+))? *(\\*/)?$",<br>
                                    llvm::Regex::IgnoreCase),<br>
     -      ShortNamespaceLines(Options.get("ShortNamespaceLines", 1u)),<br>
     -      SpacesBeforeComments(Options.get("SpacesBeforeComments",<br>
     -                                       Name.startswith("google") ? 2u : 1u)) {}<br>
     +      ShortNamespaceLines(*Options.get("ShortNamespaceLines", 1u)),<br>
     +      SpacesBeforeComments(*Options.get("SpacesBeforeComments",<br>
     +                                        Name.startswith("google") ? 2u : 1u)) {}<br>
</blockquote></div><br><br>
</div></div>