[PATCH] D23421: [Clang-tidy] CERT-MSC53-CPP (checker for std namespace modification)

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this check! There is a slight problem, however, in that the check as-written will flag false positives because there are times when it is permissible to modify the std namespace. The important bit of the CERT requirement is "Do not add declarations or definitions to the standard namespaces std or posix, or to a namespace contained therein, except for a template specialization that depends on a user-defined type that meets the standard library requirements for the original template." So the part that's missing from this check is excluding template specializations that depend on user-defined types. For instance, the following should be valid:

  namespace std {
  template <>
  struct is_error_code_enum<llvm::object::object_error> : std::true_type {};
  }

(This comes from Error.h in LLVM.)


================
Comment at: clang-tidy/cert/CERTTidyModule.cpp:37-38
@@ -35,2 +36,4 @@
     // DCL
+    CheckFactories.registerCheck<DontModifyStdNamespaceCheck>(
+        "cert-msc53-cpp");
     CheckFactories.registerCheck<VariadicFunctionDefCheck>(
----------------
Should put this under a `// MSC` comment rather than the `// DCL` one.

================
Comment at: clang-tidy/cert/DontModifyStdNamespaceCheck.cpp:33
@@ +32,3 @@
+
+  diag(Nmspc->getLocation(), "Modification of " + namespaceName +
+                                 " namespace can result to undefined behavior");
----------------
Should use format specifiers instead of building the string manually. e.g., using %0. Also, diagnostics should start with a lowercase letter and "to" should be "in".

================
Comment at: clang-tidy/cert/DontModifyStdNamespaceCheck.h:19
@@ +18,3 @@
+
+/// Modification of the std or posix namespace can result to undefined behavior.
+/// This checker warns for such modifications.
----------------
s/to/in

================
Comment at: docs/clang-tidy/checks/list.rst:12
@@ -11,2 +11,3 @@
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
+   cert-msc53-cpp
    cert-env33-c
----------------
Should keep this in alphabetical order.


Repository:
  rL LLVM

https://reviews.llvm.org/D23421