[PATCH] D17434: [Clang-tidy] Make readability-container-size-empty working with STL string

[David Blaikie via cfe-commits]

On Fri, Feb 19, 2016 at 10:38 AM, Eugene Zelenko <eugene.zelenko at gmail.com>
wrote:

> On Fri, Feb 19, 2016 at 10:32 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >
> >
> > On Fri, Feb 19, 2016 at 10:25 AM, Eugene Zelenko via cfe-commits
> > <cfe-commits at lists.llvm.org> wrote:
> >>
> >> Eugene.Zelenko added a comment.
> >>
> >> Also I agree that testing is good idea, it doesn't make sense in current
> >> incarnation which test only vector and set and only with containers'
> code
> >> snippets instead of real implementations. I wrote about last issue in
> >> cfe-dev, but idea was rejected.
> >
> >
> > Not sure I quite follow why stub implementations wouldn't be a valid test
> > strategy here?
>
> I don't think that yet another stub will change anything.
>

It will test the change you're making - as we test any change (where
reasonably possible), no?


>
> >> Other issue is how to avoid code duplication for test conditions.
> Probably
> >> tests should be generated from templates for each container or should
> >> contain container definition and include shared test body.
> >
> >
> > Are the tests really long enough to have a lot of duplication worth
> > reducing/removing? Seems like the code should be pretty short...
>
> My concern is about duplication test conditions. I found some false
> positives in this check couple of month ago, so test conditions were
> updated. It'll be pain to update them in 17 files.


17 files? Seems there's only one test file for this check,
test/clang-tidy/readability-container-size-empty.cpp

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160219/5430456a/attachment-0001.html>