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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
