<div dir="ltr">Hi Rafael,<div><br></div><div>> <span style="font-size:13px">The main thing I like about the diagnostic system is that it lets us</span></div><span style="font-size:13px">> differentiate two related but independent concepts:</span><br style="font-size:13px">><br style="font-size:13px"><span style="font-size:13px">> * Giving the human using the program diagnostics about what went wrong.</span><br style="font-size:13px"><span style="font-size:13px">> * Propagating an error to the caller so that the upper library layer</span><br style="font-size:13px"><span style="font-size:13px">> can handle it or pass it up the stack.</span><div><span style="font-size:13px"><br></span></div><div>I don't think these are really independent. Whether or not you need to emit a diagnostic depends on whether a caller can handle the corresponding error, which isn't something you know at the point where the error is raised. That's the idea behind the 'log' method on TypedErrorInfo: It lets you transform meaningful information about the problem into a log message *after* you know whether it can be handled or not. Recoverable errors can be logged (if the client wants to do so), but they don't have to be.</div><div><br></div><div>Using TypedError for diagnostics also means one fewer friction point when composing library code: Rather than having to agree on both the error return type and the diagnostic context type you only have to get agreement on the error return type.</div><div><br></div><div><span style="font-size:13px">> That is way more than what we need to pass to the caller. In fact, the</span><br style="font-size:13px"><span style="font-size:13px">> only cases we need to differentiate are "this is not a bitcode file at</span><br style="font-size:13px"><span style="font-size:13px">> all" and "the file is broken", which we do with the following enum +</span><br style="font-size:13px"><span style="font-size:13px">> std::error_code.</span><br></div><div><br></div><div>By contrast, in my system this would be:</div><div><br></div><div><font face="monospace, monospace">class InvalidBitcodeSignature : TypedErrorInfo<InvalidBitcodeSignature> {};</font></div><div><font face="monospace, monospace">class CorruptedBitcode : TypedErrorInfo<CorruptedBitcode> {</font></div><div><font face="monospace, monospace">public:</font></div><div><font face="monospace, monospace">  CorruptedBitcode(std::string Msg) : Msg(Msg) {}</font></div><div><font face="monospace, monospace">  void log(raw_ostream &OS) const override { OS << Msg; }</font></div><div><font face="monospace, monospace">private:</font></div><div><font face="monospace, monospace">  std::string Msg; </font></div><div><font face="monospace, monospace">};</font></div><div><br></div><div>Once you factor out the need to define an error category, I suspect my system actually requires less code, not that either of them require much.</div><div><br></div><div>> <span style="font-size:13px">Note that with error_code one can define arbitrary error categories.</span></div><div><br></div><div>The problem with error categories is that they're limited to static groupings of kinds of errors, but the user might need dynamic information to recover.</div><div><br></div><div>Consider someone who wants to use libObject to write an object-file repair tool: A "bad_symbol" error code tells you what went wrong, but not where or why, so it's not much use in attempting a fix. On the other hand "bad_symbol { idx = 10 }" gives you enough information to pop up a dialog, ask the user what the symbol at index 10 should be, then re-write that symbol table entry.</div><div><br></div><div><span style="font-size:13px">> Other advantages are:</span><br style="font-size:13px">><br style="font-size:13px"><span style="font-size:13px">> * It is easy to use fatal error in simple programs by just using a</span><br style="font-size:13px"><span style="font-size:13px">> diganostic handler that exits.</span><br style="font-size:13px"><span style="font-size:13px">> * Very debugger friendly. It is easy to put a breakpoint at the diag handler.</span><br></div><div><span style="font-size:13px"><br></span></div><div>This proposal provides the same advantages. I noted the second point in the original RFC. The first is easily implemented using an idiom I've seen in llvm-objdump and elsewhere:</div><div><br></div><div><font face="monospace, monospace">void check(TypedError Err) {</font></div><div><font face="monospace, monospace">  if (!Err)</font></div><div><font face="monospace, monospace">    return;</font></div><div><font face="monospace, monospace"><br></font></div><div><font face="monospace, monospace">  logAllUnhandledErrors(std::move(Err), errs(), "<tool name>");</font></div><div><font face="monospace, monospace">  exit(1);</font></div><div><font face="monospace, monospace">}</font></div><div><span style="font-size:13px"><font face="monospace, monospace"><br></font></span></div><div><span style="font-size:13px"><font face="monospace, monospace">...</font></span></div><div><font face="monospace, monospace">TypedErrorOr<Result> R = doSomething();</font></div><div><font face="monospace, monospace">check(R.takeError());</font></div><div><font face="monospace, monospace">... *R;</font></div><div><br></div><div>Side-note: you can actually go one better and replace this idiom with:</div><div><br></div><div><font face="monospace, monospace">template <typename T></font></div><div><font face="monospace, monospace">T check(TypedErrorOr<T> ValOrErr) {</font></div><div><font face="monospace, monospace">  if (!ValOrErr) {</font></div><div><font face="monospace, monospace">    logAllUnhandledErrors(ValOrErr.takeError(), errs(), "<tool name>");</font></div><div><font face="monospace, monospace">    exit(1);</font></div><div><font face="monospace, monospace">  }</font></div><div><font face="monospace, monospace">  return std::move(*ValOrErr);</font></div><div><font face="monospace, monospace">}</font></div><div><span style="font-size:13px"><font face="monospace, monospace"><br></font></span></div><div><span style="font-size:13px"><font face="monospace, monospace">...</font></span></div><div><span style="font-size:13px"><font face="monospace, monospace">Result R = check(doSomething());</font></span></div><div><span style="font-size:13px"><font face="monospace, monospace">...</font></span></div><div><span style="font-size:13px"><br></span></div><div>Mind you, this new idiom works equally well for ErrorOr/std::error_code. </div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">> </span><span style="font-size:13px">So, could you achieve your objectives by:</span></div>><br style="font-size:13px"><span style="font-size:13px">> * Moving the diagnostic handling code to Support so that all of llvm can use it.</span><br style="font-size:13px"><span style="font-size:13px">> * Defining TypedError as a wrapper over std::error_code with a</span><br style="font-size:13px"><span style="font-size:13px">> destructor that verifies the error was handled?</span><div><span style="font-size:13px"><br></span></div><div>Using TypedError as a wrapper for std::error_code would prevent errors from be silently dropped, which is a huge step forward. (Though TypedError would be a misnomer - we'd need a new name. :)</div><div><br></div><div>Using a diagnostic handler would allow richer diagnostics from libObject, but it supports a narrower class of error-recovery than my proposal. I'm inclined to favor my proposal as a strictly more general solution.</div><div><br></div><div>Cheers,</div><div>Lang.</div><div> </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 9, 2016 at 11:12 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 3 February 2016 at 02:33, Lang Hames via llvm-dev<br>
<span class=""><<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br>
> Hi Mehdi,<br>
><br>
>> If you subclass a diagnostic right now, isn’t the RTTI information<br>
>> available to the handler, which can then achieve the same dispatching /<br>
>> custom handling per type of diagnostic?<br>
>> (I’m not advocating the diagnostic system, which I found less convenient<br>
>> to use than what you are proposing)<br>
><br>
> I have to confess I haven't looked at the diagnostic classes closely. I'll<br>
> take a look and get back to you on this one. :)<br>
<br>
</span>The main thing I like about the diagnostic system is that it lets us<br>
differentiate two related but independent concepts:<br>
<br>
* Giving the human using the program diagnostics about what went wrong.<br>
* Propagating an error to the caller so that the upper library layer<br>
can handle it or pass it up the stack.<br>
<br>
For example, when given a broken bitcode file we are able to produce<br>
the diagnostic "Unknown attribute kind (52)" which shows exactly what<br>
we are complaining about. We are able to do that because the<br>
diagnostic is produced close to the spot where the error is detected.<br>
<br>
That is way more than what we need to pass to the caller. In fact, the<br>
only cases we need to differentiate are "this is not a bitcode file at<br>
all" and "the file is broken", which we do with the following enum +<br>
std::error_code.<br>
<br>
  enum class BitcodeError { InvalidBitcodeSignature = 1, CorruptedBitcode };<br>
<br>
So we use the context to produce a diagnostic, but the error doesn't<br>
need to store it. Compare that with what we had before r225562.<br>
<br>
Note that with error_code one can define arbitrary error categories.<br>
<br>
Other advantages are:<br>
<br>
* It is easy to use fatal error in simple programs by just using a<br>
diganostic handler that exits.<br>
* Very debugger friendly. It is easy to put a breakpoint at the diag handler.<br>
<br>
Given that distinction, what I agree that is really missing is<br>
infrastructure to make sure std::error_code is handled and for<br>
filtering it.<br>
<br>
So, could you achieve your objectives by:<br>
<br>
* Moving the diagnostic handling code to Support so that all of llvm can use it.<br>
* Defining TypedError as a wrapper over std::error_code with a<br>
destructor that verifies the error was handled?<br>
<br>
Thanks,<br>
Rafael<br>
</blockquote></div><br></div>