[PATCH] D89651: [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: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