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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth 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
+
----------------
gbencze wrote:
> 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? 
Rather splitting. Duplication might be painfull in the future.


================
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);
----------------
gbencze wrote:
> 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;
> };
> ```
Yes. Because it is only tested for the current archtiecture the buildbots will probably fail on it, because they test many architectures.

I think it is necessary to specify the arch in the RUN-line (`%t -- -- -target x86_64-unknown-unknown`) at the end of it.

And yes: Another test-file would be great to demonstrate that it works as expected.


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

https://reviews.llvm.org/D71973