<div class="gmail_quote">On Mon, Apr 25, 2011 at 8:38 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Ping. ¬†Any comments on this patch?</blockquote><div><br></div><div>I like the general direction of this patch, but I have one nit, and one more serious comment.</div><div><br></div><div>nit: You should switch back to the enum for the ArgumentKind in your interface rather than exposing the raw 'char' storage type. This may be made irrelevant by the major comment though....</div>
<div><br></div><div>My big issue is similar to Sebastian's. This slows down the *building* of diagnostics. It would be much better to do all of this logic in the Diagnostic class.</div><div><br></div><div>To give a concrete reason, SFINAE: we sometimes build many many diagnostics merely to detect the presence of them, discard a particular type with those diagnostics, and then begin semantically analyzing something else. We don't want to pay for this type of formatting logic in that case. We only want to pay for it when we are *emitting* the diagnostic.</div>
<div><br></div><div>Another reason; after we've hit the max diagnostic emitted limit, it would be good to cut any and every corner we can to speed things up.</div><div><br></div><div>Finally, I think it will be more semantically clear. No need to funnel an enum through so many layers, etc.</div>
</div>