[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9e1a4ce0b55d: [clang-tidy] Fix for 
bugprone-sizeof-expression PR57167 (authored by chrish_ericsson_atx).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -147,8 +147,8 @@
   const auto StructAddrOfExpr = unaryOperator(
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
-  const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  const auto PointerToStructType =
+  hasUnqualifiedDesugaredType(pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 453442.
chrish_ericsson_atx added a comment.

Removed `// FIXME` comments, per @aaron.ballman's feedback.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -147,8 +147,8 @@
   const auto StructAddrOfExpr = unaryOperator(
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
-  const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  const auto PointerToStructType =
+  hasUnqualifiedDesugaredType(pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

aaron.ballman wrote:
> mizvekov wrote:
> > aaron.ballman wrote:
> > > mizvekov wrote:
> > > > chrish_ericsson_atx wrote:
> > > > > mizvekov wrote:
> > > > > > chrish_ericsson_atx wrote:
> > > > > > > njames93 wrote:
> > > > > > > > mizvekov wrote:
> > > > > > > > > Is this change really desirable, or should we put a FIXME 
> > > > > > > > > here?
> > > > > > > > Not warning on these cases seems like a pretty big red flag, 
> > > > > > > > especially the `MyStruct *`.
> > > > > > > Thank you for your comment!  I'm not sure I fully agree that this 
> > > > > > > is a red flag.  I'm inclined to think that whether or not there 
> > > > > > > should be a warning on `MyStruct *` or `PMyStruct` is a pretty 
> > > > > > > subjective.  These are both very common idioms, and are 
> > > > > > > meaningful.  I do appreciate the perspective that `MyStruct *` is 
> > > > > > > just one character different from `MyStruct`, and as such, it 
> > > > > > > might be a typo to ask for sizeof the pointer, when you really 
> > > > > > > wanted sizeof the struct.  But the counter-argument (accidentally 
> > > > > > > asking for sizeof the struct when you really wanted the pointer 
> > > > > > > size) is just as valid-- and the checker obviously cannot warn in 
> > > > > > > that case.   
> > > > > > > 
> > > > > > > I am perfectly fine with kicking the can down the road a bit by 
> > > > > > > replacing the discarded `// CHECK-MESSAGES` directive with a `// 
> > > > > > > FIXME`, as @mizvekov suggested.  I expect that when someone 
> > > > > > > circles back to really deeply reconsider the semantics of this 
> > > > > > > checker, there will be a number of changes (other existing 
> > > > > > > warnings dropped, warnings for new and missed cases added, much 
> > > > > > > better sync between C and C++, as well as intentional 
> > > > > > > consideration of things like __typeof__ (in it's various forms) 
> > > > > > > and decltype, rather than the haphazard coarse-grain matching 
> > > > > > > that seems to be going on now.
> > > > > > I agree with this patch only in so far that:
> > > > > > 
> > > > > > * This is effectively a partial revert of the changes made in 
> > > > > > https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
> > > > > > * The checker was pretty much buggy to begin with.
> > > > > > * That change significantly increased the amount of patterns we 
> > > > > > would accept, but at the same time the existing tests did not pick 
> > > > > > that up and this was not carefully considered.
> > > > > > * It seems unreasonable that there is effectively no way to shut 
> > > > > > this warning up per site, except by a NOLINT directive.
> > > > > > 
> > > > > > If the amount of false positives is so high now that this check is 
> > > > > > unusable, then this is just a question of reverting to a less 
> > > > > > broken state temporarily.
> > > > > > 
> > > > > > But otherwise we can't leave it in the reverted state either. 
> > > > > > Without that change to use the canonical type or something similar, 
> > > > > > there is no reason to suppose any of these test cases would work at 
> > > > > > all as clang evolves and we improve the quality of implementation 
> > > > > > wrt type sugar retention.
> > > > > @mizvekov, I agree with your reasoning for saying "we can't leave it 
> > > > > in the reverted state either".  But I'm not sure how to ensure that 
> > > > > this gets the needed attention.  Should we just file a separate PR on 
> > > > > github to track the needed refactoring?  I do not believe I'll have 
> > > > > the bandwidth to look at this in the next few months.
> > > > > 
> > > > > In the meantime, assuming the bots are happy with patchset 2, I'll 
> > > > > land this as-is later today.
> > > > > 
> > > > > Thank you very much for your feedback!
> > > > Oh please wait for others to review, I don't think landing today is 
> > > > reasonable!
> > > > 
> > > > I am not really a stakeholder for this checker except for that original 
> > > > change.
> > > > 
> > > > I would advise for you to wait for another more responsible reviewer to 
> > > > accept as well before merging, unless this gets stale for a long time 
> > > > and no one else seems to be interested.
> > > > Thank you for your comment! I'm not sure I fully agree that this is a 
> > > > red flag. I'm inclined to think that whether or not there should be a 
> > > > warning on MyStruct * or PMyStruct is a pretty subjective. These are 
> > > > both very common idioms, and are meaningful. I do appreciate the 
> > > > perspective that 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from removing some comments now that we figured out what's going on. 
Please hold off on landing for a day or two in case @njames93 has other 
opinions though.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:235-236
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(MyStruct*);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
+  sum += sizeof(PMyStruct);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
   sum += sizeof(PS);





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

mizvekov wrote:
> aaron.ballman wrote:
> > mizvekov wrote:
> > > chrish_ericsson_atx wrote:
> > > > mizvekov wrote:
> > > > > chrish_ericsson_atx wrote:
> > > > > > njames93 wrote:
> > > > > > > mizvekov wrote:
> > > > > > > > Is this change really desirable, or should we put a FIXME here?
> > > > > > > Not warning on these cases seems like a pretty big red flag, 
> > > > > > > especially the `MyStruct *`.
> > > > > > Thank you for your comment!  I'm not sure I fully agree that this 
> > > > > > is a red flag.  I'm inclined to think that whether or not there 
> > > > > > should be a warning on `MyStruct *` or `PMyStruct` is a pretty 
> > > > > > subjective.  These are both very common idioms, and are meaningful. 
> > > > > >  I do appreciate the perspective that `MyStruct *` is just one 
> > > > > > character different from `MyStruct`, and as such, it might be a 
> > > > > > typo to ask for sizeof the pointer, when you really wanted sizeof 
> > > > > > the struct.  But the counter-argument (accidentally asking for 
> > > > > > sizeof the struct when you really wanted the pointer size) is just 
> > > > > > as valid-- and the checker obviously cannot warn in that case.   
> > > > > > 
> > > > > > I am perfectly fine with kicking the can down the road a bit by 
> > > > > > replacing the discarded `// CHECK-MESSAGES` directive with a `// 
> > > > > > FIXME`, as @mizvekov suggested.  I expect that when someone circles 
> > > > > > back to really deeply reconsider the semantics of this checker, 
> > > > > > there will be a number of changes (other existing warnings dropped, 
> > > > > > warnings for new and missed cases added, much better sync between C 
> > > > > > and C++, as well as intentional consideration of things like 
> > > > > > __typeof__ (in it's various forms) and decltype, rather than the 
> > > > > > haphazard coarse-grain matching that seems to be going on now.
> > > > > I agree with this patch only in so far that:
> > > > > 
> > > > > * This is effectively a partial revert of the changes made in 
> > > > > https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
> > > > > * The checker was pretty much buggy to begin with.
> > > > > * That change significantly increased the amount of patterns we would 
> > > > > accept, but at the same time the existing tests did not pick that up 
> > > > > and this was not carefully considered.
> > > > > * It seems unreasonable that there is effectively no way to shut this 
> > > > > warning up per site, except by a NOLINT directive.
> > > > > 
> > > > > If the amount of false positives is so high now that this check is 
> > > > > unusable, then this is just a question of reverting to a less broken 
> > > > > state temporarily.
> > > > > 
> > > > > But otherwise we can't leave it in the reverted state either. Without 
> > > > > that change to use the canonical type or something similar, there is 
> > > > > no reason to suppose any of these test cases would work at all as 
> > > > > clang evolves and we improve the quality of implementation wrt type 
> > > > > sugar retention.
> > > > @mizvekov, I agree with your reasoning for saying "we can't leave it in 
> > > > the reverted state either".  But I'm not sure how to ensure that this 
> > > > gets the needed attention.  Should we just file a separate PR on github 
> > > > to track the needed refactoring?  I do not believe I'll have the 
> > > > bandwidth to look at this in the next few months.
> > > > 
> > > > In the meantime, assuming the bots are happy with patchset 2, I'll land 
> > > > this as-is later today.
> > > > 
> > > > Thank you very much for your feedback!
> > > Oh 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

aaron.ballman wrote:
> mizvekov wrote:
> > chrish_ericsson_atx wrote:
> > > mizvekov wrote:
> > > > chrish_ericsson_atx wrote:
> > > > > njames93 wrote:
> > > > > > mizvekov wrote:
> > > > > > > Is this change really desirable, or should we put a FIXME here?
> > > > > > Not warning on these cases seems like a pretty big red flag, 
> > > > > > especially the `MyStruct *`.
> > > > > Thank you for your comment!  I'm not sure I fully agree that this is 
> > > > > a red flag.  I'm inclined to think that whether or not there should 
> > > > > be a warning on `MyStruct *` or `PMyStruct` is a pretty subjective.  
> > > > > These are both very common idioms, and are meaningful.  I do 
> > > > > appreciate the perspective that `MyStruct *` is just one character 
> > > > > different from `MyStruct`, and as such, it might be a typo to ask for 
> > > > > sizeof the pointer, when you really wanted sizeof the struct.  But 
> > > > > the counter-argument (accidentally asking for sizeof the struct when 
> > > > > you really wanted the pointer size) is just as valid-- and the 
> > > > > checker obviously cannot warn in that case.   
> > > > > 
> > > > > I am perfectly fine with kicking the can down the road a bit by 
> > > > > replacing the discarded `// CHECK-MESSAGES` directive with a `// 
> > > > > FIXME`, as @mizvekov suggested.  I expect that when someone circles 
> > > > > back to really deeply reconsider the semantics of this checker, there 
> > > > > will be a number of changes (other existing warnings dropped, 
> > > > > warnings for new and missed cases added, much better sync between C 
> > > > > and C++, as well as intentional consideration of things like 
> > > > > __typeof__ (in it's various forms) and decltype, rather than the 
> > > > > haphazard coarse-grain matching that seems to be going on now.
> > > > I agree with this patch only in so far that:
> > > > 
> > > > * This is effectively a partial revert of the changes made in 
> > > > https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
> > > > * The checker was pretty much buggy to begin with.
> > > > * That change significantly increased the amount of patterns we would 
> > > > accept, but at the same time the existing tests did not pick that up 
> > > > and this was not carefully considered.
> > > > * It seems unreasonable that there is effectively no way to shut this 
> > > > warning up per site, except by a NOLINT directive.
> > > > 
> > > > If the amount of false positives is so high now that this check is 
> > > > unusable, then this is just a question of reverting to a less broken 
> > > > state temporarily.
> > > > 
> > > > But otherwise we can't leave it in the reverted state either. Without 
> > > > that change to use the canonical type or something similar, there is no 
> > > > reason to suppose any of these test cases would work at all as clang 
> > > > evolves and we improve the quality of implementation wrt type sugar 
> > > > retention.
> > > @mizvekov, I agree with your reasoning for saying "we can't leave it in 
> > > the reverted state either".  But I'm not sure how to ensure that this 
> > > gets the needed attention.  Should we just file a separate PR on github 
> > > to track the needed refactoring?  I do not believe I'll have the 
> > > bandwidth to look at this in the next few months.
> > > 
> > > In the meantime, assuming the bots are happy with patchset 2, I'll land 
> > > this as-is later today.
> > > 
> > > Thank you very much for your feedback!
> > Oh please wait for others to review, I don't think landing today is 
> > reasonable!
> > 
> > I am not really a stakeholder for this checker except for that original 
> > change.
> > 
> > I would advise for you to wait for another more responsible reviewer to 
> > accept as well before merging, unless this gets stale for a long time and 
> > no one else seems to be interested.
> > Thank you for your comment! I'm not sure I fully agree that this is a red 
> > flag. I'm inclined to think that whether or not there should be a warning 
> > on MyStruct * or PMyStruct is a pretty subjective. These are both very 
> > common idioms, and are meaningful. I do appreciate the perspective that 
> > MyStruct * is just one character different from MyStruct, and as such, it 
> > might be a typo to ask for sizeof the pointer, when you really wanted 
> > sizeof the struct. But the counter-argument (accidentally asking for sizeof 
> > the struct when you really wanted the pointer size) is just as valid-- and 
> > the checker obviously cannot warn in that case.
> 
> I agree with @njames93 that 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c:42
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));

Based on the documentation, I would expect this one to be diagnosed.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

mizvekov wrote:
> chrish_ericsson_atx wrote:
> > mizvekov wrote:
> > > chrish_ericsson_atx wrote:
> > > > njames93 wrote:
> > > > > mizvekov wrote:
> > > > > > Is this change really desirable, or should we put a FIXME here?
> > > > > Not warning on these cases seems like a pretty big red flag, 
> > > > > especially the `MyStruct *`.
> > > > Thank you for your comment!  I'm not sure I fully agree that this is a 
> > > > red flag.  I'm inclined to think that whether or not there should be a 
> > > > warning on `MyStruct *` or `PMyStruct` is a pretty subjective.  These 
> > > > are both very common idioms, and are meaningful.  I do appreciate the 
> > > > perspective that `MyStruct *` is just one character different from 
> > > > `MyStruct`, and as such, it might be a typo to ask for sizeof the 
> > > > pointer, when you really wanted sizeof the struct.  But the 
> > > > counter-argument (accidentally asking for sizeof the struct when you 
> > > > really wanted the pointer size) is just as valid-- and the checker 
> > > > obviously cannot warn in that case.   
> > > > 
> > > > I am perfectly fine with kicking the can down the road a bit by 
> > > > replacing the discarded `// CHECK-MESSAGES` directive with a `// 
> > > > FIXME`, as @mizvekov suggested.  I expect that when someone circles 
> > > > back to really deeply reconsider the semantics of this checker, there 
> > > > will be a number of changes (other existing warnings dropped, warnings 
> > > > for new and missed cases added, much better sync between C and C++, as 
> > > > well as intentional consideration of things like __typeof__ (in it's 
> > > > various forms) and decltype, rather than the haphazard coarse-grain 
> > > > matching that seems to be going on now.
> > > I agree with this patch only in so far that:
> > > 
> > > * This is effectively a partial revert of the changes made in 
> > > https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
> > > * The checker was pretty much buggy to begin with.
> > > * That change significantly increased the amount of patterns we would 
> > > accept, but at the same time the existing tests did not pick that up and 
> > > this was not carefully considered.
> > > * It seems unreasonable that there is effectively no way to shut this 
> > > warning up per site, except by a NOLINT directive.
> > > 
> > > If the amount of false positives is so high now that this check is 
> > > unusable, then this is just a question of reverting to a less broken 
> > > state temporarily.
> > > 
> > > But otherwise we can't leave it in the reverted state either. Without 
> > > that change to use the canonical type or something similar, there is no 
> > > reason to suppose any of these test cases would work at all as clang 
> > > evolves and we improve the quality of implementation wrt type sugar 
> > > retention.
> > @mizvekov, I agree with your reasoning for saying "we can't leave it in the 
> > reverted state either".  But I'm not sure how to ensure that this gets the 
> > needed attention.  Should we just file a separate PR on github to track the 
> > needed refactoring?  I do not believe I'll have the bandwidth to look at 
> > this in the next few months.
> > 
> > In the meantime, assuming the bots are happy with patchset 2, I'll land 
> > this as-is later today.
> > 
> > Thank you very much for your feedback!
> Oh please wait for others to review, I don't think landing today is 
> reasonable!
> 
> I am not really a stakeholder for this checker except for that original 
> change.
> 
> I would advise for you to wait for another more responsible reviewer to 
> accept as well before merging, unless this gets stale for a long time and no 
> one else seems to be interested.
> Thank you for your comment! I'm not sure I fully agree that this is a red 
> flag. I'm inclined to think that whether or not there should be a warning on 
> MyStruct * or PMyStruct is a pretty subjective. These are both very common 
> idioms, and are meaningful. I do appreciate the perspective that MyStruct * 
> is just one character different from MyStruct, and as such, it might be a 
> typo to ask for sizeof the pointer, when you really wanted sizeof the struct. 
> But the counter-argument (accidentally asking for sizeof the struct when you 
> 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 453286.
chrish_ericsson_atx added a comment.

Corrected git-clang-format issue.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -232,10 +232,8 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(MyStruct*);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
+  sum += sizeof(PMyStruct);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -147,8 +147,8 @@
   const auto StructAddrOfExpr = unaryOperator(
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
-  const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  const auto PointerToStructType =
+  hasUnqualifiedDesugaredType(pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -232,10 +232,8 @@
   // CHECK-MESSAGES: 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Sounds fair.  I had taken your acceptance of the change as a green light.  :)  
TBH, the acceptance came much faster than I'd expected-- even though this is a 
trivial and low-risk change, I expected it to sit for at least several days.  
I'll plan to wait a few more days before landing it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

chrish_ericsson_atx wrote:
> mizvekov wrote:
> > chrish_ericsson_atx wrote:
> > > njames93 wrote:
> > > > mizvekov wrote:
> > > > > Is this change really desirable, or should we put a FIXME here?
> > > > Not warning on these cases seems like a pretty big red flag, especially 
> > > > the `MyStruct *`.
> > > Thank you for your comment!  I'm not sure I fully agree that this is a 
> > > red flag.  I'm inclined to think that whether or not there should be a 
> > > warning on `MyStruct *` or `PMyStruct` is a pretty subjective.  These are 
> > > both very common idioms, and are meaningful.  I do appreciate the 
> > > perspective that `MyStruct *` is just one character different from 
> > > `MyStruct`, and as such, it might be a typo to ask for sizeof the 
> > > pointer, when you really wanted sizeof the struct.  But the 
> > > counter-argument (accidentally asking for sizeof the struct when you 
> > > really wanted the pointer size) is just as valid-- and the checker 
> > > obviously cannot warn in that case.   
> > > 
> > > I am perfectly fine with kicking the can down the road a bit by replacing 
> > > the discarded `// CHECK-MESSAGES` directive with a `// FIXME`, as 
> > > @mizvekov suggested.  I expect that when someone circles back to really 
> > > deeply reconsider the semantics of this checker, there will be a number 
> > > of changes (other existing warnings dropped, warnings for new and missed 
> > > cases added, much better sync between C and C++, as well as intentional 
> > > consideration of things like __typeof__ (in it's various forms) and 
> > > decltype, rather than the haphazard coarse-grain matching that seems to 
> > > be going on now.
> > I agree with this patch only in so far that:
> > 
> > * This is effectively a partial revert of the changes made in 
> > https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
> > * The checker was pretty much buggy to begin with.
> > * That change significantly increased the amount of patterns we would 
> > accept, but at the same time the existing tests did not pick that up and 
> > this was not carefully considered.
> > * It seems unreasonable that there is effectively no way to shut this 
> > warning up per site, except by a NOLINT directive.
> > 
> > If the amount of false positives is so high now that this check is 
> > unusable, then this is just a question of reverting to a less broken state 
> > temporarily.
> > 
> > But otherwise we can't leave it in the reverted state either. Without that 
> > change to use the canonical type or something similar, there is no reason 
> > to suppose any of these test cases would work at all as clang evolves and 
> > we improve the quality of implementation wrt type sugar retention.
> @mizvekov, I agree with your reasoning for saying "we can't leave it in the 
> reverted state either".  But I'm not sure how to ensure that this gets the 
> needed attention.  Should we just file a separate PR on github to track the 
> needed refactoring?  I do not believe I'll have the bandwidth to look at this 
> in the next few months.
> 
> In the meantime, assuming the bots are happy with patchset 2, I'll land this 
> as-is later today.
> 
> Thank you very much for your feedback!
Oh please wait for others to review, I don't think landing today is reasonable!

I am not really a stakeholder for this checker except for that original change.

I would advise for you to wait for another more responsible reviewer to accept 
as well before merging, unless this gets stale for a long time and no one else 
seems to be interested.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

mizvekov wrote:
> chrish_ericsson_atx wrote:
> > njames93 wrote:
> > > mizvekov wrote:
> > > > Is this change really desirable, or should we put a FIXME here?
> > > Not warning on these cases seems like a pretty big red flag, especially 
> > > the `MyStruct *`.
> > Thank you for your comment!  I'm not sure I fully agree that this is a red 
> > flag.  I'm inclined to think that whether or not there should be a warning 
> > on `MyStruct *` or `PMyStruct` is a pretty subjective.  These are both very 
> > common idioms, and are meaningful.  I do appreciate the perspective that 
> > `MyStruct *` is just one character different from `MyStruct`, and as such, 
> > it might be a typo to ask for sizeof the pointer, when you really wanted 
> > sizeof the struct.  But the counter-argument (accidentally asking for 
> > sizeof the struct when you really wanted the pointer size) is just as 
> > valid-- and the checker obviously cannot warn in that case.   
> > 
> > I am perfectly fine with kicking the can down the road a bit by replacing 
> > the discarded `// CHECK-MESSAGES` directive with a `// FIXME`, as @mizvekov 
> > suggested.  I expect that when someone circles back to really deeply 
> > reconsider the semantics of this checker, there will be a number of changes 
> > (other existing warnings dropped, warnings for new and missed cases added, 
> > much better sync between C and C++, as well as intentional consideration of 
> > things like __typeof__ (in it's various forms) and decltype, rather than 
> > the haphazard coarse-grain matching that seems to be going on now.
> I agree with this patch only in so far that:
> 
> * This is effectively a partial revert of the changes made in 
> https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
> * The checker was pretty much buggy to begin with.
> * That change significantly increased the amount of patterns we would accept, 
> but at the same time the existing tests did not pick that up and this was not 
> carefully considered.
> * It seems unreasonable that there is effectively no way to shut this warning 
> up per site, except by a NOLINT directive.
> 
> If the amount of false positives is so high now that this check is unusable, 
> then this is just a question of reverting to a less broken state temporarily.
> 
> But otherwise we can't leave it in the reverted state either. Without that 
> change to use the canonical type or something similar, there is no reason to 
> suppose any of these test cases would work at all as clang evolves and we 
> improve the quality of implementation wrt type sugar retention.
@mizvekov, I agree with your reasoning for saying "we can't leave it in the 
reverted state either".  But I'm not sure how to ensure that this gets the 
needed attention.  Should we just file a separate PR on github to track the 
needed refactoring?  I do not believe I'll have the bandwidth to look at this 
in the next few months.

In the meantime, assuming the bots are happy with patchset 2, I'll land this 
as-is later today.

Thank you very much for your feedback!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 453022.
chrish_ericsson_atx added a comment.

Added missing newline and added // FIXME comments, per reviewer comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -232,10 +232,8 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(MyStruct*);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
+  sum += sizeof(PMyStruct);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -148,7 +148,7 @@
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
   const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -232,10 +232,8 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

chrish_ericsson_atx wrote:
> njames93 wrote:
> > mizvekov wrote:
> > > Is this change really desirable, or should we put a FIXME here?
> > Not warning on these cases seems like a pretty big red flag, especially the 
> > `MyStruct *`.
> Thank you for your comment!  I'm not sure I fully agree that this is a red 
> flag.  I'm inclined to think that whether or not there should be a warning on 
> `MyStruct *` or `PMyStruct` is a pretty subjective.  These are both very 
> common idioms, and are meaningful.  I do appreciate the perspective that 
> `MyStruct *` is just one character different from `MyStruct`, and as such, it 
> might be a typo to ask for sizeof the pointer, when you really wanted sizeof 
> the struct.  But the counter-argument (accidentally asking for sizeof the 
> struct when you really wanted the pointer size) is just as valid-- and the 
> checker obviously cannot warn in that case.   
> 
> I am perfectly fine with kicking the can down the road a bit by replacing the 
> discarded `// CHECK-MESSAGES` directive with a `// FIXME`, as @mizvekov 
> suggested.  I expect that when someone circles back to really deeply 
> reconsider the semantics of this checker, there will be a number of changes 
> (other existing warnings dropped, warnings for new and missed cases added, 
> much better sync between C and C++, as well as intentional consideration of 
> things like __typeof__ (in it's various forms) and decltype, rather than the 
> haphazard coarse-grain matching that seems to be going on now.
I agree with this patch only in so far that:

* This is effectively a partial revert of the changes made in 
https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
* The checker was pretty much buggy to begin with.
* That change significantly increased the amount of patterns we would accept, 
but at the same time the existing tests did not pick that up and this was not 
carefully considered.
* It seems unreasonable that there is effectively no way to shut this warning 
up per site, except by a NOLINT directive.

If the amount of false positives is so high now that this check is unusable, 
then this is just a question of reverting to a less broken state temporarily.

But otherwise we can't leave it in the reverted state either. Without that 
change to use the canonical type or something similar, there is no reason to 
suppose any of these test cases would work at all as clang evolves and we 
improve the quality of implementation wrt type sugar retention.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

njames93 wrote:
> mizvekov wrote:
> > Is this change really desirable, or should we put a FIXME here?
> Not warning on these cases seems like a pretty big red flag, especially the 
> `MyStruct *`.
Thank you for your comment!  I'm not sure I fully agree that this is a red 
flag.  I'm inclined to think that whether or not there should be a warning on 
`MyStruct *` or `PMyStruct` is a pretty subjective.  These are both very common 
idioms, and are meaningful.  I do appreciate the perspective that `MyStruct *` 
is just one character different from `MyStruct`, and as such, it might be a 
typo to ask for sizeof the pointer, when you really wanted sizeof the struct.  
But the counter-argument (accidentally asking for sizeof the struct when you 
really wanted the pointer size) is just as valid-- and the checker obviously 
cannot warn in that case.   

I am perfectly fine with kicking the can down the road a bit by replacing the 
discarded `// CHECK-MESSAGES` directive with a `// FIXME`, as @mizvekov 
suggested.  I expect that when someone circles back to really deeply reconsider 
the semantics of this checker, there will be a number of changes (other 
existing warnings dropped, warnings for new and missed cases added, much better 
sync between C and C++, as well as intentional consideration of things like 
__typeof__ (in it's various forms) and decltype, rather than the haphazard 
coarse-grain matching that seems to be going on now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Please can you run git clang-format over the patch to keep the pre-merge bot 
happy.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

mizvekov wrote:
> Is this change really desirable, or should we put a FIXME here?
Not warning on these cases seems like a pretty big red flag, especially the 
`MyStruct *`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov accepted this revision.
mizvekov added a comment.
This revision is now accepted and ready to land.

LGTM once minor nits are fixed. I am not opposed to getting this back to where 
it was, we can take a more careful look later at what this checker should be 
doing instead.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c:63
+}
\ No newline at end of file


Missing newline here.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

Is this change really desirable, or should we put a FIXME here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131926/new/

https://reviews.llvm.org/D131926

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx created this revision.
chrish_ericsson_atx added reviewers: mizvekov, baloghadamsoftware, njames93, 
cfe-commits.
Herald added subscribers: carlosgalvezp, rnkovacs, xazax.hun.
Herald added a project: All.
chrish_ericsson_atx requested review of this revision.
Herald added a project: clang-tools-extra.

This addresses a change in behavior of the bugprone-sizeof-expression
checker after upstream commit 15f3cd6bfc6 
, which 
cleaned up
ElaboratedType sugaring in the AST.  This restores (mostly) the
beahvior of the checker prior to that commit, which may or may not have
been consistent with the intent of the checker, but at least gave a
tolerable level of what users would consider false positives.

  

Bug: https://github.com/llvm/llvm-project/issues/57167


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -148,7 +148,7 @@
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
   const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: