<div dir="ltr">+ std::list<QualType> BaseClassTypes;<br><div><br></div><div>This will allocate memory for every node; it would be better to use a SmallVector here (and use a depth-first search rather than a breadth-first one so you're only modifying one end of your list).</div><div><br></div><div>+ for (auto &CurType : BaseClassTypes) {<br></div><div><br></div><div>It's a bit scary to be iterating over your container while modifying it (even though it's actually correct in this case). Alternative idiom:</div><div><br></div><div>SmallVector<QualType, 8> BaseClassTypes;</div><div>BaseClassTypes.push_back(...);</div><div>while (!BaseClassTypes.empty()) {</div><div> QualType T = BaseClassTypes.pop_back_val();</div><div> // ... maybe push back some values.</div><div>}</div><div><br></div><div><div>+ auto I = std::find_if(</div><div>+ HandledTypes.begin(), HandledTypes.end(),</div></div><div><br></div><div>This is still quadratic-time; maybe use a DenseMap with the canonical QualType plus a "was it a pointer" flag as a key?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 13, 2014 at 9:33 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Nov 12, 2014 at 5:05 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> I think it's not too hard to avoid the quadratic performance here. Algorithm<br>
> outline:<br>
><br>
> Start with empty map of type -> handlers<br>
> For each catch handler for a type T:<br>
> For each base class U of T (including T itself), most-derived-first:<br>
> If we have a handler for U in our map, warn, break<br>
> Add handler for T to our map<br>
><br>
> (This is still quadratic if you have struct A<n> : A<n-1>, and catch A<0>,<br>
> A<1>, ..., A<n> in that order, but in that case I think your approach is<br>
> cubic (because IsDerivedFrom is itself linear in the number of base<br>
> classes.)<br>
<br>
</span>I've reworked the patch to implement your algorithm. Thanks!<br>
<span class=""><br>
><br>
> Also, I'm not sure you're correctly handling this case:<br>
><br>
> try { ... }<br>
> catch (Base *) {}<br>
> catch (Derived &) {}<br>
><br>
> We shouldn't warn here, but I think you might, because you strip off a<br>
> top-level * or & quite early. I'd also like to see some test coverage for<br>
> catch parameters with cv-qualifiers.<br>
<br>
</span>Correct, that was handled incorrectly before, but has been fixed in<br>
this patch. I've added some further tests to make sure of it.<br>
<br>
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> On Tue, Oct 28, 2014 at 7:23 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> I noticed that this warning was a bit anachronistic, so I've done some<br>
>> updates. This patch now gives the warning a new group (GCC does not<br>
>> have a group for this, so I've created -Wexceptions), and now prints<br>
>> the QualType directly instead of forcing it through a string.<br>
>><br>
>> ~Aaron<br>
>><br>
>> On Mon, Oct 27, 2014 at 9:00 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> wrote:<br>
>> > Here is a reworked version of the same patch -- one thing to note is<br>
>> > that this already had a FIXME in the code regarding it being quadratic<br>
>> > performance. This patch retains the note about that issue, but I am<br>
>> > not too worried about the performance as this is per-try block. If<br>
>> > performance is truly questionable, we can always use the checker-based<br>
>> > approach instead.<br>
>> ><br>
>> > I've removed the TypeWithHandler helper class as it's essentially just<br>
>> > a std::pair with special logic for operator<. Since we have to perform<br>
>> > the quadratic check for derivative classes, it doesn't make sense to<br>
>> > then loop a second time over the same (sorted) data just for a simple<br>
>> > comparison of QualTypes. This makes my patch a performance regression<br>
>> > for testing duplicate handlers -- I do not have numbers regarding<br>
>> > whether this is actually a measurable regression for realistic test<br>
>> > cases or not, but my gut reaction based on our assumption of 8<br>
>> > handlers being a reasonable number suggests this likely isn't<br>
>> > perceptible.<br>
>> ><br>
>> > ~Aaron<br>
>> ><br>
>> > On Mon, Oct 27, 2014 at 7:40 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> > wrote:<br>
>> >> On Mon, Oct 27, 2014 at 7:38 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> >> wrote:<br>
>> >>> On Mon, Oct 27, 2014 at 4:35 PM, Aaron Ballman<br>
>> >>> <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> >>> wrote:<br>
>> >>>><br>
>> >>>> On Mon, Oct 27, 2014 at 7:30 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>><br>
>> >>>> wrote:<br>
>> >>>> > Nifty! But do you think this is cheap enough for a general compiler<br>
>> >>>> > warning? It certainly doesn't depend on the analyzer's<br>
>> >>>> > path-sensitive<br>
>> >>>> > analysis, so it's mostly just how much we care about the cost of<br>
>> >>>> > isDerivedFrom.<br>
>> >>>><br>
>> >>>> This should be relatively inexpensive, so it may make sense as a<br>
>> >>>> general compiler warning if others feel that's a better approach.<br>
>> >>><br>
>> >>><br>
>> >>> Yes, I think this is a good candidate for an (on-by-default) compiler<br>
>> >>> warning.<br>
>> >><br>
>> >> Then I'll rework this, thanks!<br>
>> >><br>
>> >> ~Aaron<br>
><br>
><br>
</div></div></blockquote></div><br></div>