[PATCH] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze marked an inline comment as done.
gbencze added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:45
+
+  for (unsigned int i = 0; i < 2; ++i) {
+    const Expr *ArgExpr = CE->getArg(i);
----------------
aaron.ballman wrote:
> The lint warning here is actually correct, which is a lovely change of pace.
Changed to `ArgIndex`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:70
+      if (ComparedBits.hasValue() && *ComparedBits >= PointeeSize &&
+          !Ctx.hasUniqueObjectRepresentations(PointeeQualifiedType)) {
+        diag(CE->getBeginLoc(),
----------------
aaron.ballman wrote:
> Note that this may produce false positives as the list of objects with unique representations is not complete. For instance, it doesn't handle _Complex or _Atomic types, etc.
Yes, this could definitely cause some false positives. I'm not sure how we could easily fix it thought, especially if they are in some nested record.
Do you think this is a serious issue?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:73
+             "comparing object representation of type %0 which does not have "
+             "unique object representations; consider comparing the values "
+             "manually")
----------------
aaron.ballman wrote:
> unique object representations -> a unique object representation
> 
> WDYT about:
> consider comparing the values manually -> consider comparing %select{the values|the members of the object}0 manually
> 
> to make it more clear that these cases are different:
> ```
> memcmp(&some_float, &some_other_float, sizeof(float));
> memcmp(&some_struct, &some_other_struct, sizeof(some_struct));
> ```
> The use of "values" make it sound a bit like the user should be able to do `if (some_struct == some_other_struct)` to me.
I wasn't entirely happy with the wording either; changed it according to your suggestions. 


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp:2
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown -m32
+
----------------
aaron.ballman wrote:
> Wouldn't picking a 32-bit target suffice instead of `-m32`? e.g., `-target i386-unknown-unknown`
To be fully honest, I have no idea, I saw some other tests using `-m32` so I decided to copy that. 


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c:121
+  memcmp(&a, &b, sizeof(int)); // no-warning: not comparing entire object
+  memcmp(&a, &b, 2 * sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of
----------------
aaron.ballman wrote:
> Just to make it obvious, I think a test like this would also be handy:
> ```
> struct Whatever {
>   int i[2];
>   char c;
> };
> 
> struct Whatever one, two;
> memcmp(&one, &two, 2 * sizeof(int)); // Shouldn't warn either
> ```
> which brings up a pathological case that I have no idea how it should behave:
> ```
> struct Whatever {
>   int i[2];
>   int : 0; // What now?!
> };
> 
> struct Whatever one, two;
> memcmp(&one, &two, 2 * sizeof(int)); // Warn? Don't Warn? Cry?
> ```
Added two new test cases for these: `Test_TrailingPadding2` and `Bitfield_TrailingUnnamed`. 
Purely empirically the latter one seems to have a size of `2*sizeof(int)`, so I assume there is no need to warn there. 


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp:229
+  // consider comparing the values manually
+}
+} // namespace alignment
----------------
aaron.ballman wrote:
> Another case we should be careful of is template instantiations:
> ```
> template <typename Ty>
> void func(const Ty *o1, const Ty *o2) {
>   memcmp(o1, o2, sizeof(Ty));
> }
> ```
> We don't want to diagnose that unless it's been instantiated with types that are a problem.
Thanks, I added two new tests for these. I also made a minor fix in `registerMatchers` to make sure I don't try to evaluate a dependant expression. 


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

https://reviews.llvm.org/D89651