hokein added a comment.
Thanks, I have committed for you.
Repository:
rL LLVM
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL329073: [clang-tidy] Check for sizeof that call functions
(authored by hokein, committed by ).
Herald added subscribers: llvm-commits, klimek.
Changed prior to commit:
pfultz2 updated this revision to Diff 140790.
pfultz2 added a comment.
I have rebased.
https://reviews.llvm.org/D44231
Files:
clang-tidy/bugprone/SizeofExpressionCheck.cpp
clang-tidy/bugprone/SizeofExpressionCheck.h
docs/clang-tidy/checks/bugprone-sizeof-expression.rst
hokein added a comment.
pfultz2@, could you rebase this patch? The check has been moved to bugprone.
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hokein added a comment.
Sorry for the delay -- everyone was out because of the long Easter weekend.
I'll commit for you.
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
pfultz2 added a comment.
Is someone able to merge in my changes?
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
pfultz2 added a comment.
> Do you need someone to submit this for you?
Yes
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
alexfh accepted this revision.
alexfh added a comment.
LG. Do you need someone to submit this for you?
Comment at: test/clang-tidy/misc-sizeof-expression.cpp:17
+enum E { E_VALUE = 0 };
+
aaron.ballman wrote:
> Can you add a C++11 test case using `enum
pfultz2 added a comment.
So, I've added tests for class enums and bols.
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
pfultz2 updated this revision to Diff 139649.
https://reviews.llvm.org/D44231
Files:
clang-tidy/misc/SizeofExpressionCheck.cpp
clang-tidy/misc/SizeofExpressionCheck.h
docs/clang-tidy/checks/misc-sizeof-expression.rst
test/clang-tidy/misc-sizeof-expression.cpp
Index:
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
LG modulo other reviewers' open comments.
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
aaron.ballman added inline comments.
Comment at: test/clang-tidy/misc-sizeof-expression.cpp:17
+enum E { E_VALUE = 0 };
+
Can you add a C++11 test case using `enum class` -- I am worried that the
`isInteger()` matcher will return false for that type.
Also, I
pfultz2 marked an inline comment as done.
pfultz2 added a comment.
I have reworded the documentation. Hopefully it is clearer.
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
pfultz2 added a comment.
> As this patch can catch some mistakes, I'm fine with checking it in. I agree
> that it is reasonable to write normal code like sizeof(func_call()) (not
> false positive), maybe set the option to false by default?
I have disabled it by default. We can decide later if
pfultz2 updated this revision to Diff 138791.
https://reviews.llvm.org/D44231
Files:
clang-tidy/misc/SizeofExpressionCheck.cpp
clang-tidy/misc/SizeofExpressionCheck.h
docs/clang-tidy/checks/misc-sizeof-expression.rst
test/clang-tidy/misc-sizeof-expression.cpp
Index:
aaron.ballman added a comment.
In https://reviews.llvm.org/D44231#1039888, @hokein wrote:
> As this patch can catch some mistakes, I'm fine with checking it in. I agree
> that it is reasonable to write normal code like `sizeof(func_call())` (not
> false positive), maybe set the option to
hokein added a comment.
As this patch can catch some mistakes, I'm fine with checking it in. I agree
that it is reasonable to write normal code like `sizeof(func_call())` (not
false positive), maybe set the option to `false` by default?
https://reviews.llvm.org/D44231
pfultz2 added a comment.
So I've updated the documentation on this. Are there any other updates needed
for this to get it merged in?
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
pfultz2 updated this revision to Diff 137822.
https://reviews.llvm.org/D44231
Files:
clang-tidy/misc/SizeofExpressionCheck.cpp
clang-tidy/misc/SizeofExpressionCheck.h
docs/clang-tidy/checks/misc-sizeof-expression.rst
test/clang-tidy/misc-sizeof-expression.cpp
Index:
pfultz2 added a comment.
> I don't have a script for it. I've used "bear" with at least some of those
> projects because they use makefiles rather than cmake
> (https://github.com/rizsotto/Bear). I'm not tied to those projects
> specifically, either, so if you have a different corpus you'd
aaron.ballman added a comment.
In https://reviews.llvm.org/D44231#1032782, @pfultz2 wrote:
> > Again, that only works for C++ and not C.
>
> Typedef has always worked in C.
This is true.
I think what I'm struggling to say is: I don't think this is a common pattern
in either C or C++. It's
pfultz2 added a comment.
> Again, that only works for C++ and not C.
Typedef has always worked in C.
> Did it report any true positives that would need correcting?
Not for LLVM, but it has in other projects like I mentioned.
> Can you check some other large repos (both C++ and C), such as:
aaron.ballman added a comment.
In https://reviews.llvm.org/D44231#1032011, @pfultz2 wrote:
> > If the return type of foo() is changed, then the use for s1 will be
> > automatically updated
>
> But usually you write it as:
>
> using foo_return = uint16_t;
> foo_return foo();
> ...
>
pfultz2 added a comment.
> If the return type of foo() is changed, then the use for s1 will be
> automatically updated
But usually you write it as:
using foo_return = uint16_t;
foo_return foo();
...
size_t s1 = sizeof(foo());
size_t s2 = sizeof(foo_return);
So you just update the
aaron.ballman added a comment.
In https://reviews.llvm.org/D44231#1031503, @pfultz2 wrote:
> > Can you point to some real world code that would benefit from this check?
>
> Yes, here:
>
> https://github.com/ROCmSoftwarePlatform/MIOpen/blob/master/src/convolution.cpp#L184
>
> It is erroneously
pfultz2 added a comment.
> Can you point to some real world code that would benefit from this check?
Yes, here:
https://github.com/ROCmSoftwarePlatform/MIOpen/blob/master/src/convolution.cpp#L184
It is erroneously calling `sizeof(yDesc.GetType())`. The `GetType` function
returns an enum that
aaron.ballman added a comment.
In https://reviews.llvm.org/D44231#1031380, @pfultz2 wrote:
> > Can you elaborate a bit more about this?
>
> This catches problems when calling `sizeof(f())` when `f` returns an
> integer(or enum). This is because, most likely, the integer represents the
> type
pfultz2 added a comment.
> Can you elaborate a bit more about this?
This catches problems when calling `sizeof(f())` when `f` returns an integer(or
enum). This is because, most likely, the integer represents the type to be
chosen, however, `sizeof` is being computed for an integer and not the
alexfh added inline comments.
Comment at: clang-tidy/misc/SizeofExpressionCheck.cpp:220
+ Result.Nodes.getNodeAs("sizeof-integer-call")) {
+diag(E->getLocStart(), "suspicious usage of 'sizeof(expr)' to an integer");
} else if (const auto *E =
hokein added a comment.
Can you elaborate a bit more about this? I think we also need to update the
check document (adding proper section of this new behavior, and the new option).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44231
lebedev.ri added a comment.
Please do bother to prefix with the appropriate `[prefix]`, set the appropriate
repo, and tags!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44231
___
cfe-commits mailing list
31 matches
Mail list logo
