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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:30
+                             uint64_t ComparedBits, uint64_t &TotalSize) {
+  const auto IsNotEmptyBase = [](const CXXBaseSpecifier &Base) {
+    return !Base.getType()->getAsCXXRecordDecl()->isEmpty();
----------------
Drop top-level `const` qualifiers on locals (it's not a style we typically use), elsewhere as well.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:82-83
+
+static bool hasPadding(const ASTContext &Ctx, const RecordDecl *RD,
+                       uint64_t ComparedBits) {
+  assert(RD && RD->isCompleteDefinition() &&
----------------
I am surprised that we have to do this much manual work, I would have expected this to be something that `RecordLayout` would handle for us. I am a bit worried about this manual work being split in a few places because we're likely to get it wrong in at least one of them. For instance, this doesn't seem to be taking into account things like `[[no_unique_address]]` or alignment when considering the layout of fields.

I sort of wonder if we should try using the `RecordLayout` class to do this work instead, even if that requires larger changes.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:130
+    const Type *PointeeType = ArgType->getPointeeOrArrayElementType();
+    assert(PointeeType != nullptr && "PointeeType should always be available.");
+
----------------
Can you add a test case like this:
```
struct S {
  operator void *() const;
};

S s;
memcmp(s, s, sizeof(s));
```
to ensure that this assertion doesn't trigger?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:133-134
+    if (PointeeType->isRecordType()) {
+      const RecordDecl *RD = PointeeType->getAsRecordDecl()->getDefinition();
+      if (RD != nullptr) {
+        if (const auto *CXXDecl = dyn_cast<CXXRecordDecl>(RD)) {
----------------
Sink the declaration into the if conditional to limit its scope.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:140
+                 "type %0; consider using a comparison operator instead")
+                << PointeeType->getAsTagDecl()->getQualifiedNameAsString();
+            break;
----------------
You can pass in `PointeeType->getAsTagDecl()` -- the diagnostic printer will automatically get the correct name from any `NamedDecl`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:148
+                                  "consider comparing the fields manually")
+              << PointeeType->getAsTagDecl()->getQualifiedNameAsString();
+          break;
----------------
Same here as above.


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

https://reviews.llvm.org/D71973