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

[Gabor Bencze via Phabricator via cfe-commits]

gbencze marked 18 inline comments as done.
gbencze added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp:1
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t
+
----------------
JonasToth wrote:
> We should explicitly test, that the check runs under C-code, either with two run-lines or with separate test files (my preference).
Should I duplicate the tests that are legal C or try to split it up so that only c++ specific code is tested in this one? 


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp:3
+
+typedef unsigned long long size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
----------------
JonasToth wrote:
> I think the test could be sensitive to cpu-architectures.
> If you could bring up a case that is e.g. problematic on X86, but not on anything else (powerpc, or arm or anything, 64bit vs 32bit) and demonstrate with tests that such cases are caught as well this would be great. I see no reason it wouldn't.
> 
> That would maybe justify another alias into `portability`, as this is an issue in that case.
I may have misunderstood you but the check currently only warns if you're comparing padding on the current compilation target. 
Or do you mean adding another test file that is compiled e.g. with -m32 and testing if calling memcmp on the following doesn't warn there but does on 64bit?
```
struct S {
    int x;
    int* y;
};
```


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

https://reviews.llvm.org/D71973