<div dir="ltr">On 30 January 2013 19:15, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Strange indentation in SortUndefinedInternals::operator(). (I assume<br>
this was clang-format obstinately refusing to indent anything within a<br>
namespace?)<br></blockquote><div><br></div><div style>Doh, thanks. Clang-format was not involved.</div><div style><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


Other than that, LGTM<br></blockquote><div><br></div><div style>Thanks!! Committed in r174034.</div><div><br></div><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 Wed, Jan 30, 2013 at 6:43 PM, Nick Lewycky <<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>> wrote:<br>
> On 29 January 2013 18:58, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>><br>
>> On Tue, Jan 29, 2013 at 6:50 PM, Nick Lewycky <<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>> wrote:<br>
>> > On 29 January 2013 14:16, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>> >><br>
>> >> Hi Nick,<br>
>> >><br>
>> >> Your operator< might not be a total order -- it's possible for uses of<br>
>> >> two functions to have the same source location if at least one of them<br>
>> >> is implicitly called (as part of an implicit conversion, for<br>
>> >> instance). Perhaps you could break ties by comparing source locations<br>
>> >> of the NamedDecl?<br>
>> ><br>
>> ><br>
>> > Done. UndefinedInternal now holds a SourceManager* (the alternative<br>
>> > being a<br>
>> > second FullSourceLoc for the decl).<br>
>> ><br>
>> >> Also, please capitalize the added local variable names (I think these<br>
>> >> were introduced by reverting a previous change?), and remove the<br>
>> >> redundant braces in SemaDecl.cpp.<br>
>> ><br>
>> ><br>
>> > Done.<br>
>> ><br>
>> > Updated patch attached<br>
>><br>
>> Looks fine, though it'd be nice to factor the common code out of the<br>
>> diagnostic and serialization paths.<br>
><br>
><br>
> Yep. Updated patch attached!<br>
><br>
> Nick<br>
><br>
>> >> On Tue, Jan 29, 2013 at 4:04 AM, Nick Lewycky <<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>><br>
>> >> wrote:<br>
>> >> > Currently UndefinedInternals grows an entry each time an internal<br>
>> >> > function<br>
>> >> > is ODR-used before it is defined, and this mapping is filtered only<br>
>> >> > when<br>
>> >> > diagnostics are emitted.<br>
>> >> ><br>
>> >> > This patch shrinks that list by removing entries as functions are<br>
>> >> > defined.<br>
>> >> > Because that's a performance sensitive path, we test to see whether<br>
>> >> > there<br>
>> >> > even is a previous function declaration, and if so that it was<br>
>> >> > ODR-used,<br>
>> >> > before doing the lookup that would remove the entry. Secondly, this<br>
>> >> > patch<br>
>> >> > applies the same filtering used when emitting diagnostics to emitting<br>
>> >> > PCH.<br>
>> >> ><br>
>> >> > A large chunk of this patch is reverting the switch to MapVector in<br>
>> >> > r173538,<br>
>> >> > as MapVector does not (and can not) support efficient removals.<br>
>> >> ><br>
>> >> > Please review!<br>
>> >> ><br>
>> >> > Nick<br>
>> >> ><br>
>> >> > _______________________________________________<br>
>> >> > cfe-commits mailing list<br>
>> >> > <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>