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
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
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
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
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
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
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:
>
>
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
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
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(
+
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
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(
> +
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
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
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;
>
17 matches
Mail list logo