[clang-tidy] Extend cert-oop57-cpp to check non-zero memset values

[gamesh411]

Summary

Clang Tidy check cert-oop57-cpp now checks for arbitrary-valued integer
literals in memset expressions containing non-trivially
default-constructible instances. Previously it only checked 0 values.

Note that the first non-compliant example of
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP57-CPP.+Prefer+special+member+functions+and+overloaded+operators+to+C+Standard+Library+functions requires this.

A comparative analysis of OS projects is currently running to see the impact of this change.

[steakhal]

Looks reasonable to me.

[gamesh411]

{F23163926}
There is no change in the results as far as these OS projects are concerned.

[njames93]

Please upload diff with full context.
Can you add a note about this in the release notes.

As a side point I'm not sure this change really follows what the rule is trying to enforce. The rule is about not using std::memset to reinitialise objects that aren't trivial. Having said that limiting it to only 0 does seem a little restrictive,

[gamesh411]

Added a release note
Also generated the full context (arcanist could validate the site certificate, that's why I had to resort to manual diff creation. Was there a certificate change on the reviews.llmv.org site maybe?)

[steakhal]

In D126186#3530967, @gamesh411 wrote:

Was there a certificate change on the reviews.llmv.org site maybe?)

Yes, AFAIK.

Please redo the measurement, it doesn't look right.

[njames93]

This diff looks like it's rooted on the clang-tools-extra directory which is why the pre-merge bot is failing to build.

[aaron.ballman]

In D126186#3530938, @njames93 wrote:

As a side point I'm not sure this change really follows what the rule is trying to enforce. The rule is about not using std::memset to reinitialise objects that aren't trivial. Having said that limiting it to only 0 does seem a little restrictive,

I think this change makes sense for the rule as it's written, per "Do not use std::memset() to initialize an object of nontrivial class type as it may not properly initialize the value representation of the object." The noncompliant example was using zero initialization, but there's nothing inherently special about 0 except for how commonly used it is as a default value.

[gamesh411]

Add full diff with arcanist

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

201 ↗

(On Diff #431348)

Please sort entries in section alphabetically.

202 ↗

(On Diff #431348)

Please fix double space and enclose memset into double back-ticks.

[gamesh411]

fix Release Notes

[gamesh411]

Remove literal checking from the matcher for memset as well

There is no change in the result set on open source projects even without
restricting the matches to literals.

IMO this is more in line with the rule as it's written as @aaron.ballman mentioned.

Updated the ReleaseNotes to reflect this.

[aaron.ballman]

LGTM!

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

160 ↗

(On Diff #432868)

Please use double back-ticks for language constructs.

[gamesh411]

Thanks for the quick review!
Fixed the double backtick in the release notes as well.

clang-tidy/cert/NonTrivialTypesLibcMemoryCallsCheck.cpp

return allOf(argumentCountIs(3),
                  hasArgument(0, IsStructPointer(RecordConstraint, true)),
                  hasArgument(1, SecondArg), hasArgument(2, IsRecordSizeOf));
   };
 
   Finder->addMatcher(
       callExpr(callee(namedDecl(hasAnyName(
                    utils::options::parseListPair(BuiltinMemSet, MemSetNames)))),
                ArgChecker(unless(isTriviallyDefaultConstructible()),
-                          expr(integerLiteral(equals(0)))))
+                          expr(integerLiteral())))
           .bind("lazyConstruct"),
       this);
   Finder->addMatcher(
       callExpr(callee(namedDecl(hasAnyName(
                    utils::options::parseListPair(BuiltinMemCpy, MemCpyNames)))),
                ArgChecker(unless(isTriviallyCopyable()), IsStructPointer()))
           .bind("lazyCopy"),
       this);

docs/ReleaseNotes.rst

 - Fixed a false positive in :doc:`readability-non-const-parameter
   <clang-tidy/checks/readability-non-const-parameter>` when the parameter is
   referenced by an lvalue.
 
 - Expanded :doc:`readability-simplify-boolean-expr
   <clang-tidy/checks/readability-simplify-boolean-expr>` to simplify expressions
   using DeMorgan's Theorem.
 
+- Made :doc:`cert-oop57-cpp <clang-tidy/checks/cert-oop57-cpp>` more sensitive
+  by checking for non-zero integer literal  memset arguments as well.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

test/clang-tidy/checkers/cert-oop57-cpp.cpp

void baz(const NonTrivial &Other) {
   NonTrivial Data;
   mymemset(&Data, 0, sizeof(Data));
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling 'mymemset' on a non-trivially default constructible class is undefined
   mymemcpy(&Data, &Other, sizeof(Data));
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling 'mymemcpy' on a non-trivially copyable class is undefined
   mymemcmp(&Data, &Other, sizeof(Data));
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: consider using comparison operators instead of calling 'mymemcmp'
 }
+
+void nonNullSetValue() {
+  NonTrivial Data;
+  // Check non-null-valued second argument.
+  std::memset(&Data, 1, sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling 'memset' on a non-trivially default constructible class is undefined
+}