aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:76 + "values|the members of the object}1 " + "manually") + << PointeeQualifiedType << (PointeeType->isRecordType() ? 1 : 0); ---------------- It looks like this part of the string literal can be moved up a line. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:70 + if (ComparedBits.hasValue() && *ComparedBits >= PointeeSize && + !Ctx.hasUniqueObjectRepresentations(PointeeQualifiedType)) { + diag(CE->getBeginLoc(), ---------------- gbencze wrote: > 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? I don't think it's a serious issue except for possibly the `_Atomic` case in C code. It may be worth testing that scenario explicitly and putting a FIXME comment on the test case. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst:13 +See also: +`OOP57-CPP. Prefer special member functions and overloaded operators to C Standard Library functions +<https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP57-CPP.+Prefer+special+member+functions+and+overloaded+operators+to+C+Standard+Library+functions>`_ ---------------- May also want to mention EXP62-CPP which is similarly related to OOP57-CPP -- but the docs should be clear that this isn't a check for those rules, just that it has some partial overlap. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst:16 + +**Case 2: Type with no unique object representations** + ---------------- Type -> Types representations -> representation ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst:18 + +Objects with the same value may not have the same object representaions. +This may be caused by padding or floating-point types. ---------------- representaions -> representation ================ 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 ---------------- gbencze wrote: > 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. Thanks for the new test cases, and I think that emergent behavior is reasonable. 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