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
cla
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
d
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(
+ has(expr(hasType(hasC
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
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,
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(
> + has(ex
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 a
alexfh updated this revision to Diff 34451.
alexfh marked 3 inline comments as done.
alexfh added a comment.
Don't complain on the ARRAYSIZE() pattern (where
sizeof(container) is used as a denominator).
http://reviews.llvm.org/D12759
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/Mis
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;
I don't think we need to remove anything beyond the most external pair of
parenthes
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 size of the "
> > +
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;
>
> I d
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 SizeOfLoc = SizeOf->getLocStart();
>> > + auto Diag
aaron.ballman accepted this revision.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
http://reviews.llvm.org/D12759
___
cfe-commits mailing list
cfe-commits@lists.
aaron.ballman added a comment.
This appears to have broken one of the bots:
http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/15065
http://reviews.llvm.org/D12759
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.or
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:
>
> http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/
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 silenced
I've also found a bunch of similar cases in our codebase, and I'm trying to
figure out whether the pattern can be narrowed down to just dangerous
cases. If we don't find a way to do so, we'll probably have to resort to
"// NOLINT" to shut clang-tidy up.
On 13 Sep 2015 10:52, "Kim Gräsman" 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.
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 figure out whether
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 11:22 AM, Alexander Ko
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 with this check in a large
> codebase.
> > Turn
On Wed, Sep 16, 2015 at 5:21 PM, Alexander Kornienko wrote:
> All found results were intended usages of sizeof on containers. 100% false
> positive rate that is.
Yes, but is that 4 results in 10MM LoC, or 4000 results in 40k LoC?
;-) I guess I just don't have a good feel for how large the codebas
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
> wrote:
> > All found results were intende
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 catch real bugs.
~Aaro
23 matches
Mail list logo
