This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by gamesh411 on May 23 2022, 1:22 AM.

Details

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.

Diff Detail

Event Timeline

gamesh411 created this revision.May 23 2022, 1:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 1:22 AM
gamesh411 requested review of this revision.May 23 2022, 1:22 AM
gamesh411 added a comment.EditedMay 23 2022, 3:09 AM

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

njames93 added a comment.EditedMay 23 2022, 3:20 AM

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 updated this revision to Diff 431319.May 23 2022, 3:44 AM

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?)

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

Yes, AFAIK.

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

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

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 updated this revision to Diff 431348.May 23 2022, 5:52 AM

Add full diff with arcanist

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
208

Please sort entries in section alphabetically.

209

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

gamesh411 updated this revision to Diff 432866.May 30 2022, 1:23 AM

fix Release Notes

gamesh411 updated this revision to Diff 432868.May 30 2022, 1:37 AM

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.

This revision is now accepted and ready to land.May 31 2022, 7:32 AM
Eugene.Zelenko added inline comments.May 31 2022, 7:35 AM
clang-tools-extra/docs/ReleaseNotes.rst
160

Please use double back-ticks for language constructs.

gamesh411 marked 3 inline comments as done.May 31 2022, 11:24 PM

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