[PATCH] D32700: [clang-tidy] Add misc-suspicious-memset-usage check.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

> The existing google-runtime-memset-zero-length check is related. It finds cases when the byte count parameter is zero and offers to swap that with the fill value argument. Perhaps the two could be merged while maintaining backward compatibility through an alias.

I think, the checks should be merged, and there's no need to maintain the old behavior under the `google-runtime-memset-zero-length` alias. I'm not even sure we want to make this alias, as long as the new check isn't ridiculously noisy.

At this point I'd suggest that we go back to the idea of adding a separate module for checks that target patterns that are likely to be bugs rather than performance, readability or style issues. The name could be "bugprone" or something like this, and we could move many misc- checks there eventually.



================
Comment at: clang-tidy/misc/SuspiciousMemsetUsageCheck.cpp:21
+void SuspiciousMemsetUsageCheck::registerMatchers(MatchFinder *Finder) {
+  const auto HasCtorOrDtor =
+      eachOf(hasMethod(cxxConstructorDecl(unless(anyOf(
----------------
rnkovacs wrote:
> xazax.hun wrote:
> > I think this might not be the best approach.
> > 
> > For example, if the constructor is compiler generated, but there is a member of the class with non-trivial constructor, we still want to warn. 
> > 
> > E.g.:
> > 
> > ```
> > struct X { X() { /* something nontrivial */ } };
> > 
> > struct Y { X x; };
> > ```
> > 
> > Maybe we should check instead whether the class is a POD? Other alternative might be something like 
> > `CXXRecordDecl::hasNonTrivialDefaultConstructor`.
> So, we had a discussion yesterday that I'll try to sum up here. The root of the problem is not exactly about constructors or the object being a POD, but rather about calling `memset()` on an object that is not `TriviallyCopyable`. But as you suggested, this holds for `memcpy()` and `memmove()` as well, and might be better placed in another check.
This logic could be either here or in a separate check (covering 'memcpy', 'memmove' and friends), but it might even be reasonable to create a compiler diagnostic for this eventually (maybe after a test drive of the logic in a clang-tidy check).


https://reviews.llvm.org/D32700