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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;"
----------------
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)


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp:45
+                                    "::strcmp;";
+static constexpr llvm::StringRef EqualityComparison[] = {"operator==",
+                                                         "operator!="};
----------------
Was there a reason you were not also looking for the relational operators like `<` or `>`?


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp:55-57
+static std::vector<llvm::StringRef> toStringRefVec(const std::vector<std::string> & Items){
+  return std::vector<llvm::StringRef>(Items.begin(), Items.end());
+}
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp:75
+    MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp:102
+
+  auto IsIntZero = expr(integerLiteral(equals(0)));
+
----------------
This can be lowered into its only use.


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp:126
+  if (const auto *Caller = Result.Nodes.getNodeAs<CallExpr>("lazyConstruct")) {
+    diag(Caller->getBeginLoc(), "Calling '%0' on a non trivially default "
+                                "constructible class is undefined")
----------------
Calling -> calling
non trivially -> non-trivially

(Same comments apply to the other diagnostics.)


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.cpp:128
+                                "constructible class is undefined")
+        << getName(*Caller);
+  }
----------------
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())`


================
Comment at: clang-tools-extra/clang-tidy/cert/NotTrivialTypesLibcMemoryCallsCheck.h:18
+
+/// This check flags use of the c standard library functions 'memset', 'memcpy',
+/// 'memmove', 'strcpy', 'memcmp' and 'strcmp' on non trivial types.
----------------
c -> C


================
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:
----------------
I prefer the file to be named `NonTrivialTypesLibcMemoryCallsCheck.h` -- the `Not` looks strange to my eyes. Same for the class name.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-oop57-cpp.rst:12
+
+.. option:: MemSetNames
+
----------------
You should specify that all of these options take a semicolon-delimited list of names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72488