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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
