[PATCH] D72488: [clang-tidy] Add check for CERT-OOP57-CPP

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp:33-44
+static const char BuiltinMemSet[] = "::std::memset;"
+                                    "::memset;";
+static const char BuiltinMemCpy[] = "::std::memcpy;"
+                                    "::memcpy;"
+                                    "::std::memmove;"
+                                    "::memmove;"
+                                    "::std::strcpy;"
----------------
aaron.ballman wrote:
> 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)
I might hijack the list from bugprone-suspicous-string-compare too :)


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp:75
+    MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
----------------
aaron.ballman wrote:
> 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.
Doesn't the CPlusPlus check block ObjC. Though it may not block ObjC++???


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.h:23
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-oop57-cpp.html
+class NotTrivialTypesLibcMemoryCallsCheck : public ClangTidyCheck {
+public:
----------------
aaron.ballman wrote:
> I prefer the file to be named `NonTrivialTypesLibcMemoryCallsCheck.h` -- the `Not` looks strange to my eyes. Same for the class name.
On looking so do I


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72488/new/

https://reviews.llvm.org/D72488