aaron.ballman added a comment. In D89651#2342367 <https://reviews.llvm.org/D89651#2342367>, @gbencze wrote:
> In D89651#2338266 <https://reviews.llvm.org/D89651#2338266>, @njames93 wrote: > >> Should point out there is already a check for cert-oop57-cpp, added in >> D72488 <https://reviews.llvm.org/D72488>. Do these play nice with each other >> or should they perhaps be merged or share code? > > I would certainly expect some duplicate warnings with there two checks, but > as far as I can tell that check does not currently warn on using `memcmp` > with non-standard-layout types. > I personally would leave the exp42 and flp37 parts of this check as separate, > because those are useful in C as well. But maybe it'd be better to move the > warning for non-standard-layout types from here to the existing one. The thrust of the idea behind OOP57-CPP is that you shouldn't use C functions to do work that would normally be done via a C++ overloaded operator. e.g., don't call `memcmp()` on two structs, define the appropriate set of relational and equality operators for the type instead. It's required that you do this for non-trivial types, but it's aspirational to do it for all types. I don't think the proposed check should relate to OOP57-CPP all that much -- I see it more closely relating to EXP62-CPP instead: https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP62-CPP.+Do+not+access+the+bits+of+an+object+representation+that+are+not+part+of+the+object%27s+value+representation except that this check is currently only about comparisons and not accessing, so it's not quite perfect coverage. ================ 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); ---------------- The lint warning here is actually correct, which is a lovely change of pace. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:70 + if (ComparedBits.hasValue() && *ComparedBits >= PointeeSize && + !Ctx.hasUniqueObjectRepresentations(PointeeQualifiedType)) { + diag(CE->getBeginLoc(), ---------------- 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. ================ 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") ---------------- 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. ================ 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 + ---------------- Wouldn't picking a 32-bit target suffice instead of `-m32`? e.g., `-target i386-unknown-unknown` ================ 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 ---------------- 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? ``` ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c:198 +} + +struct PaddingAfterUnion { ---------------- How about a test where everything lines up perfectly for bit-fields? ``` struct Whatever { int i : 10; int j : 10; int k : 10; int l : 2; }; _Static_assert(sizeof(struct Whatever) == sizeof(int)); ``` ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp:229 + // consider comparing the values manually +} +} // namespace alignment ---------------- 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89651/new/ https://reviews.llvm.org/D89651 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
