Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

An update: I didn't find a single bug with this check in a large codebase. Turns out that it's rather useless. I'm inclined to kill it. On Sun, Sep 13, 2015 at 11:22 AM, Alexander Kornienko wrote: > I've also found a bunch of similar cases in our codebase, and I'm trying > to

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

On Wed, Sep 16, 2015 at 2:21 PM, Alexander Kornienko wrote: > An update: I didn't find a single bug with this check in a large codebase. > Turns out that it's rather useless. I'm inclined to kill it. How bad is the false positive rate? ~Aaron > > > On Sun, Sep 13, 2015 at

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

All found results were intended usages of sizeof on containers. 100% false positive rate that is. On 16 Sep 2015 21:23, "Aaron Ballman" wrote: > On Wed, Sep 16, 2015 at 2:21 PM, Alexander Kornienko > wrote: > > An update: I didn't find a single bug

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

It was about a hundred in a huge codebase. It's definitely manageable, but the experiment has shown that this kind of a mistake is not likely to happen. On 16 Sep 2015 23:25, "Aaron Ballman" wrote: > On Wed, Sep 16, 2015 at 5:21 PM, Alexander Kornienko

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

On Wed, Sep 16, 2015 at 5:33 PM, Alexander Kornienko wrote: > It was about a hundred in a huge codebase. It's definitely manageable, but > the experiment has shown that this kind of a mistake is not likely to > happen. Fair enough, let's axe it until we see evidence it may

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

Late to the party, but I wanted to ask: is there a way to indicate to the checker that we really *did* mean sizeof()? I think I've stumbled over code in our code base that uses sizeof(container) to report memory usage statistics and it seems valid, so it'd be nice if this checker could be

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

Indeed. But this has been fixed before I could get to it. On Thu, Sep 10, 2015 at 10:47 PM, Aaron Ballman via cfe-commits < cfe-commits@lists.llvm.org> wrote: > aaron.ballman added a comment. > > This appears to have broken one of the bots: > >

[PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

alexfh created this revision. alexfh added a reviewer: djasper. alexfh added a subscriber: cfe-commits. sizeof(some_std_string) is likely to be an error. This check finds this pattern and suggests using .size() instead. http://reviews.llvm.org/D12759 Files: clang-tidy/misc/CMakeLists.txt

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

alexfh updated this revision to Diff 34441. alexfh added a comment. Ignore template instantiations. http://reviews.llvm.org/D12759 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/SizeofContainerCheck.cpp clang-tidy/misc/SizeofContainerCheck.h

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

aaron.ballman added a subscriber: aaron.ballman. aaron.ballman added a comment. Great idea for a checker! Some comments below. Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:36 @@ +35,3 @@ + Finder->addMatcher( + expr(sizeOfExpr( +

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

aaron.ballman added a comment. I noticed a few more things, but mostly nitpicky at this point. Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:22 @@ +21,3 @@ +bool needsParens(const Expr *E) { + E = E->IgnoreImpCasts(); + if (isa(E) || isa(E)) Should we

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

alexfh updated this revision to Diff 34446. alexfh added a comment. Match a broader set of containers. Updated diagnostic message. Added tests. http://reviews.llvm.org/D12759 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/SizeofContainerCheck.cpp

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

alexfh marked 3 inline comments as done. Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:37 @@ +36,3 @@ + expr(unless(isInTemplateInstantiation()), + sizeOfExpr( + has(expr(hasType(hasCanonicalType(hasDeclaration(recordDecl( Yes,

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

On Thu, Sep 10, 2015 at 10:54 AM, Alexander Kornienko wrote: > alexfh marked 3 inline comments as done. > > > Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:37 > @@ +36,3 @@ > + expr(unless(isInTemplateInstantiation()), > + sizeOfExpr( > +

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

On Thu, Sep 10, 2015 at 12:14 PM, Alexander Kornienko wrote: > On Thu, Sep 10, 2015 at 5:22 PM, Aaron Ballman > wrote: >> >> > >> > Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:49 >> > @@ +48,3 @@ >> > + SourceLocation

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

On Thu, Sep 10, 2015 at 5:22 PM, Aaron Ballman wrote: > > > > Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:49 > > @@ +48,3 @@ > > + SourceLocation SizeOfLoc = SizeOf->getLocStart(); > > + auto Diag = diag(SizeOfLoc, "sizeof() doesn't return the

Re: [PATCH] D12759: [clang-tidy] Add misc-sizeof-container check to find sizeof() uses on stlcontainers.

On Thu, Sep 10, 2015 at 12:04 PM, Alexander Kornienko wrote: > alexfh marked 5 inline comments as done. > > > Comment at: clang-tidy/misc/SizeofContainerCheck.cpp:23 > @@ +22,3 @@ > + E = E->IgnoreImpCasts(); > + if (isa(E) || isa(E)) > +return true; >