<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 10, 2016 at 8:40 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"><div class="HOEnZb"><div class="h5">On 10 February 2016 at 11:10, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Wed, Feb 10, 2016 at 6:47 AM, Rafael Espíndola <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>><br>
> wrote:<br>
>><br>
>> >> This highlights why I think it is important to keep diagnostics and<br>
>> >> errors separate. In the solution you have there is a need to allocate<br>
>> >> a std::string, even if that is never used.<br>
>> ><br>
>> > Errors are only constructed on the error path. There is no construction<br>
>> > on<br>
>> > the success path.<br>
>><br>
>> But they are always created, even if it as error the caller wants to<br>
>> ignore. For example, you will always create a "file foo.o in bar.a is<br>
>> not a bitcode" message (or copy sufficient information for that to be<br>
>> created). With a dignostic handler no copying is needed, since the<br>
>> call happens in the context where the error is found. It is easy to<br>
>> see us in a position where a lot of context is copied because some<br>
>> client somewhere might want it.<br>
>><br>
>> So I am worried we are coding for the hypothetical and adding<br>
>> complexity. In particular, if we are going this way I think it is<br>
>> critical that your patch *removes* the existing diagnostic handlers<br>
>> (while making sure test/Bitcode/invalid.ll still passes) so that we<br>
>> don't end up with two overlapping solutions.<br>
><br>
><br>
> Removes diagnostic handlers in other parts of LLVM (& Clang) - the IR<br>
> verifier's diagnostic handling is what you're referring to here ^?<br>
><br>
> I don't think that would be an improvement - it seems like different<br>
> situations call for different tools (as I was saying yesterday on this<br>
> thread). In some places a diagnostic handler is the right tool, and in some<br>
> places error codes/results/etc are the right tool. We already live in that<br>
> world & it seems like a reasonable one (there doesn't seem to be a<br>
> fundamental conflict between our bool+std::string or error_code returns and<br>
> existing diagnostic handlers - I think they can reasonably coexist in<br>
> different parts of the codebase for different use cases)<br>
<br>
</div></div>In which case we do need a plain checked error_code. Right now we use<br>
a diagnostic handler in the BitcodeReader, but it is really easy to<br>
miss propagating an error out. We shouldn't be in a position where we<br>
have to decide to use an diagnostic handler or have checked errors. </blockquote><div><br></div><div>I don't think we are or would be in that position based on this proposal - it sounds to me like the TypedError has an implementation that wraps error_code & that one could be used in places where we want a diagnostic handler + checked errors (& as a side benefit we get hierarchical errors, which can be handy) without having to bundle all the necessary state for diagnostic emission through that path if it happens not to be the right tool for the job in some contexts.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It<br>
should be possible to have both if it is not desired that the new<br>
error handling replaces the use of diagnostic handling.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div>