<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 19, 2016 at 10:38 AM, Eugene Zelenko <span dir="ltr"><<a href="mailto:eugene.zelenko@gmail.com" target="_blank">eugene.zelenko@gmail.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 Fri, Feb 19, 2016 at 10:32 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Fri, Feb 19, 2016 at 10:25 AM, Eugene Zelenko via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Eugene.Zelenko added a comment.<br>
>><br>
>> Also I agree that testing is good idea, it doesn't make sense in current<br>
>> incarnation which test only vector and set and only with containers' code<br>
>> snippets instead of real implementations. I wrote about last issue in<br>
>> cfe-dev, but idea was rejected.<br>
><br>
><br>
> Not sure I quite follow why stub implementations wouldn't be a valid test<br>
> strategy here?<br>
<br>
</span>I don't think that yet another stub will change anything.<br></blockquote><div><br></div><div>It will test the change you're making - as we test any change (where reasonably possible), no?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
>> Other issue is how to avoid code duplication for test conditions. Probably<br>
>> tests should be generated from templates for each container or should<br>
>> contain container definition and include shared test body.<br>
><br>
><br>
> Are the tests really long enough to have a lot of duplication worth<br>
> reducing/removing? Seems like the code should be pretty short...<br>
<br>
</span>My concern is about duplication test conditions. I found some false<br>
positives in this check couple of month ago, so test conditions were<br>
updated. It'll be pain to update them in 17 files.</blockquote><div><br></div><div>17 files? Seems there's only one test file for this check, test/clang-tidy/readability-container-size-empty.cpp<br><br>It's not necessary to test every condition where the fix applies for every container, since (I assume) they're pretty orthogonal in the implementation (especially judging by the simplicity of the fix you're proposing). So simply testing that at least one of those cases works for each of the types seems reasonable to me. (the same sort of coverage that's already there for std::set, I think.</div></div></div></div>