Page MenuHomePhabricator

[clang-tidy] Add check for CERT-OOP57-CPP
ClosedPublic

Authored by njames93 on Thu, Jan 9, 5:42 PM.

Details

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp
51 ↗(On Diff #237229)

Please add check for C++.

clang-tools-extra/docs/ReleaseNotes.rst
70

Is this relevant?

72

Please separate with empty line and synchronize with first statement in documentation.

Please highlight memset, memcpy, memmove, strcpy, memcmp and strcmp with double back-ticks.

It may be reasonable to provide options to expand list of problematic functions. See bugprone-suspicious-string-compare as example.

njames93 updated this revision to Diff 237315.Fri, Jan 10, 7:06 AM
njames93 marked 4 inline comments as done.

Added a way to specify custom mem set copy and compare functions plus nit fixes

Can now use the options MemSetNames, MemCpyNames and MemCmpNames to specify custom functions to flag on

clang-tools-extra/docs/ReleaseNotes.rst
70

nope, I renamed the check left that in , will remove

Please describe options in documentation. See other checks documentation as example.

clang-tools-extra/docs/ReleaseNotes.rst
73

Please remove This check. Same in documentation.

njames93 updated this revision to Diff 237367.Fri, Jan 10, 9:49 AM

Tweaked a few documentation issues

Eugene.Zelenko added inline comments.Fri, Jan 10, 9:55 AM
clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp
75 ↗(On Diff #237367)

Unnecessary empty line.

clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst
15

I look on other checks documentation and indentation for options is 3 characters.

njames93 updated this revision to Diff 237390.Fri, Jan 10, 10:34 AM
njames93 marked 3 inline comments as done.
  • Fix format of options docs and remove unnecessary empty line
clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst
15

Please enclose memset in double back-ticks. Same for other options.

16

Please append dot. Same for other options.

17

This is tautological because extra functions assume additions to functions handled by default. Same for other options.

njames93 updated this revision to Diff 237393.Fri, Jan 10, 11:01 AM
njames93 marked 3 inline comments as done.
  • Fixed more documentation nits
clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst
17

This repeats first sentence in documentation, so it should be removed. Same in other places.

njames93 updated this revision to Diff 237422.Fri, Jan 10, 1:02 PM
  • Reworked options documentation
njames93 marked an inline comment as done.Fri, Jan 10, 1:02 PM
Eugene.Zelenko added inline comments.Fri, Jan 10, 1:14 PM
clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst
15

Somehow memset is still not in double back-ticks. Same for other options.

njames93 updated this revision to Diff 237492.Sat, Jan 11, 4:53 AM
njames93 marked an inline comment as done.

Figured out what memset you meant...

aaron.ballman added inline comments.Tue, Jan 14, 6:19 AM
clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp
33–44 ↗(On Diff #237492)

I think these should also include the w variants of the calls.

Other APIs to consider: strncmp, strncpy, and memccpy (note the extra c in the name) as all of these are part of the C standard these days.

You may also want to look through the POSIX docs to see if we should add those APIs as well (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/string.h.html)

45 ↗(On Diff #237492)

Was there a reason you were not also looking for the relational operators like < or >?

55–57 ↗(On Diff #237492)

The formatting looks off here, but I would prefer if we could find a way to remove this code entirely. It looks very dangerous (StringRef is a non-owning data type) and also seems wasteful (you need two copies of the vector in memory at once).

The way it's being used seems to be safe and functional, but this still feels unclean.

75 ↗(On Diff #237492)

I think this should probably also be disabled for ObjC. The CERT rule doesn't cover it, and I don't know enough about the "right way" to perform the comparisons there.

102 ↗(On Diff #237492)

This can be lowered into its only use.

126 ↗(On Diff #237492)

Calling -> calling
non trivially -> non-trivially

(Same comments apply to the other diagnostics.)

128 ↗(On Diff #237492)

Rather than calling getName(), you can pass in the NamedDecl* directly and the diagnostic engine handles properly printing the name. e.g. getName(*Caller) should be cast<NamedDecl>(Caller->getCalleeDecl())

clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.h
18 ↗(On Diff #237492)

c -> C

23 ↗(On Diff #237492)

I prefer the file to be named NonTrivialTypesLibcMemoryCallsCheck.h -- the Not looks strange to my eyes. Same for the class name.

clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst
13

You should specify that all of these options take a semicolon-delimited list of names.

njames93 updated this revision to Diff 238052.Tue, Jan 14, 11:30 AM
njames93 marked 13 inline comments as done.
  • Fix a few nits, extra names will follow
clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp
33–44 ↗(On Diff #237492)

I might hijack the list from bugprone-suspicous-string-compare too :)

75 ↗(On Diff #237492)

Doesn't the CPlusPlus check block ObjC. Though it may not block ObjC++???

clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.h
23 ↗(On Diff #237492)

On looking so do I

Unit tests: pass. 61850 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 updated this revision to Diff 238114.Tue, Jan 14, 2:53 PM
  • remove This check from docs... again
Eugene.Zelenko added inline comments.Tue, Jan 14, 2:57 PM
clang-tools-extra/docs/ReleaseNotes.rst
76

Please use double back-ticks to highlight function names.

clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst
7

Please use double back-ticks to highlight function names.

Unit tests: fail. 61849 tests passed, 1 failed and 781 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_class/try_lock.pass.cpp

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61850 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 marked 3 inline comments as done.Tue, Jan 14, 5:58 PM
njames93 added inline comments.
clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst
7

Thats what i get for c&p

njames93 updated this revision to Diff 238153.Tue, Jan 14, 5:58 PM
njames93 marked an inline comment as done.
  • double ticks

Unit tests: pass. 61850 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

aaron.ballman accepted this revision.Thu, Jan 16, 7:06 AM

LGTM with a few nits.

clang-tools-extra/clang-tidy/cert/NonTrivialTypesLibcMemoryCallsCheck.cpp
72–75

I'd combine these into a single if.

clang-tools-extra/clang-tidy/cert/NonTrivialTypesLibcMemoryCallsCheck.h
26

You should do this formatting change, the per-merge check is actually correct for once. Don't bother with the ones in the test files though.

This revision is now accepted and ready to land.Thu, Jan 16, 7:06 AM
njames93 updated this revision to Diff 238753.Fri, Jan 17, 5:05 AM
  • added few more functions, fix format error

Unit tests: pass. 61941 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 updated this revision to Diff 238775.Fri, Jan 17, 7:36 AM
njames93 marked an inline comment as done.
  • Fix If stmt and optimise building function name list
njames93 marked an inline comment as done.Fri, Jan 17, 7:37 AM

Unit tests: pass. 61956 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Btw, do you need me to commit this on your behalf, or have you obtained your commit privileges?

njames93 updated this revision to Diff 239153.Mon, Jan 20, 9:08 AM
  • rebase truck
This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 62027 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst
25

This should be in general documentation, not in option. Please enclose function names in double back-ticks.

Same for other similar statements below.