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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/cert/DontModifyStdNamespaceCheck.cpp:34
+
+  diag(N->getLocation(),
+       "modification of '%0' namespace can result in undefined behavior")
----------------
I think this will still trigger false positives because there are times when it is permissible to modify the std namespace outside of a system header. 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/DontModifyStdNamespaceCheck.cpp:36
+       "modification of '%0' namespace can result in undefined behavior")
+      << N->getName();
+}
----------------
No need to call `getName()` -- you can pass `N` directly. When you correct this, also remove the explicit quotes from the diagnostic (they're added automatically by the diagnostics engine).


================
Comment at: docs/clang-tidy/checks/cert-dcl58-cpp.rst:8
+behavior.
+This check warns for such modifications.
+
----------------
You should add a link to the CERT guideline this addresses. See docs/clang-tidy/checks/cert-dcl50-cpp.rst as an example.


https://reviews.llvm.org/D23421