[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Paula Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG00a57558978d: [clang-tidy] Add check 
llvmlibc-implementation-in-namespace. (authored by PaulkaToast).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818

Files:
  clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h
  clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
@@ -0,0 +1,40 @@
+// RUN: %check_clang_tidy %s llvmlibc-implementation-in-namespace %t
+
+#define MACRO_A "defining macros outside namespace is valid"
+
+class ClassB;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be declared within the '__llvm_libc' namespace
+struct StructC {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: declaration must be declared within the '__llvm_libc' namespace
+char *VarD = MACRO_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be declared within the '__llvm_libc' namespace
+typedef int typeE;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: declaration must be declared within the '__llvm_libc' namespace
+void funcF() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: declaration must be declared within the '__llvm_libc' namespace
+
+namespace namespaceG {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: '__llvm_libc' needs to be the outermost namespace
+namespace __llvm_libc{
+namespace namespaceH {
+class ClassB;
+} // namespace namespaceH
+struct StructC {};
+} // namespace __llvm_libc
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+} // namespace namespaceG
+
+// Wrapped in correct namespace.
+namespace __llvm_libc {
+// Namespaces within __llvim_libc namespace allowed.
+namespace namespaceI {
+class ClassB;
+} // namespace namespaceI
+struct StructC {};
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+extern "C" void extern_funcJ() {}
+} // namespace __llvm_libc
Index: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvmlibc-implementation-in-namespace
+
+llvmlibc-implementation-in-namespace
+
+
+Checks that all declarations in the llvm-libc implementation are within the
+correct namespace.
+
+.. code-block:: c++
+
+// Correct: implementation inside the correct namespace.
+namespace __llvm_libc {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+// Namespaces within __llvm_libc namespace are allowed.
+namespace inner{
+int localVar = 0;
+}
+// Functions with C linkage are allowed.
+extern "C" void str_fuzz(){}
+}
+
+// Incorrect: implementation not in a namespace.
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+
+// Incorrect: outer most namespace is not correct.
+namespace something_else {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -188,6 +188,7 @@
`llvm-prefer-isa-or-dyn-cast-in-conditionals `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-implementation-in-namespace `_,
`llvmlibc-restrict-system-libc-headers `_, "Yes"
`misc-definitions-in-headers `_, "Yes"
`misc-misplaced-const `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -113,6 +113,11 @@
   Flags use of the `C` standard library functions ``memset``, ``memcpy`` and
   ``memcmp`` and similar derivatives on non-trivial types.
 
+- New :doc:`llvmlibc-implementation-in-namespace
+  ` check.
+
+  Checks all llvm-libc implementation is within the correct namespace.
+
 - New :doc:`llvmlibc-restrict-system-libc-headers
   ` check.
 
Index: 

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:21
+  Finder->addMatcher(
+  decl(hasParent(translationUnitDecl()), unless(linkageSpecDecl()))
+  .bind("child_of_translation_unit"),

PaulkaToast wrote:
> aaron.ballman wrote:
> > This skips linkage spec declarations, but are there other declarations that 
> > should be similarly skipped? For instance `static_assert` declarations?
> I believe that linkage is the only exception needed, static_asserts and all 
> other declarations should also be within the namespace. 
Excellent, thank you for verifying!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:21
+  Finder->addMatcher(
+  decl(hasParent(translationUnitDecl()), unless(linkageSpecDecl()))
+  .bind("child_of_translation_unit"),

aaron.ballman wrote:
> This skips linkage spec declarations, but are there other declarations that 
> should be similarly skipped? For instance `static_assert` declarations?
I believe that linkage is the only exception needed, static_asserts and all 
other declarations should also be within the namespace. 



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:33-34
+
+  if (isa(MatchedDecl)) {
+const auto *NS = cast(MatchedDecl);
+if (NS->getName() != RequiredNamespace) {

aaron.ballman wrote:
> Instead of doing an `isa<>` followed by a `cast<>`, the more common pattern 
> is to do:
> ```
> if (const auto *NS = dyn_cast(MatchedDecl)) {
> ```
Ah thank you this looks much nicer!



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:42
+  diag(MatchedDecl->getLocation(),
+   "Please wrap implentation in '%0' namespace.")
+  << RequiredNamespace;

aaron.ballman wrote:
> They also aren't grammatically correct sentences, so the capital P and period 
> should both go. While this definitely gets points for politeness, I think a 
> more typical diagnostic might be: `declaration must be declared within the 
> '%0' namespace`
ah, alright, good call. (:



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h:35
+private:
+  std::string RequiredNamespace;
+};

aaron.ballman wrote:
> njames93 wrote:
> > This can be made const
> Will there only ever be a single namespace? Or should this be a list (for 
> instance, a main namespace and a details namespace)?
There will only be this one namespace.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst:32-35
+.. option:: RequiredNamespace
+
+The namespace that llvm-libc implementations must be wrapped in. The 
default
+is `__llvm_libc`.

aaron.ballman wrote:
> Given that this check is specific to llvm-libc, why is the option needed at 
> all?
I was concerned that maybe there would be a desire to make the check 
generalized, however it does seem quite specific so I will take your advice. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 255277.
PaulkaToast marked 13 inline comments as done.
PaulkaToast added a comment.

Addressed njames's and aaron's comments. (:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818

Files:
  clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h
  clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
@@ -0,0 +1,40 @@
+// RUN: %check_clang_tidy %s llvmlibc-implementation-in-namespace %t
+
+#define MACRO_A "defining macros outside namespace is valid"
+
+class ClassB;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be declared within the '__llvm_libc' namespace
+struct StructC {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: declaration must be declared within the '__llvm_libc' namespace
+char *VarD = MACRO_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: declaration must be declared within the '__llvm_libc' namespace
+typedef int typeE;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: declaration must be declared within the '__llvm_libc' namespace
+void funcF() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: declaration must be declared within the '__llvm_libc' namespace
+
+namespace namespaceG {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: '__llvm_libc' needs to be the outermost namespace
+namespace __llvm_libc{
+namespace namespaceH {
+class ClassB;
+} // namespace namespaceH
+struct StructC {};
+} // namespace __llvm_libc
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+} // namespace namespaceG
+
+// Wrapped in correct namespace.
+namespace __llvm_libc {
+// Namespaces within __llvim_libc namespace allowed.
+namespace namespaceI {
+class ClassB;
+} // namespace namespaceI
+struct StructC {};
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+extern "C" void extern_funcJ() {}
+} // namespace __llvm_libc
Index: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvmlibc-implementation-in-namespace
+
+llvmlibc-implementation-in-namespace
+
+
+Checks that all declarations in the llvm-libc implementation are within the
+correct namespace.
+
+.. code-block:: c++
+
+// Correct: implementation inside the correct namespace.
+namespace __llvm_libc {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+// Namespaces within __llvm_libc namespace are allowed.
+namespace inner{
+int localVar = 0;
+}
+// Functions with C linkage are allowed.
+extern "C" void str_fuzz(){}
+}
+
+// Incorrect: implementation not in a namespace.
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+
+// Incorrect: outer most namespace is not correct.
+namespace something_else {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -188,6 +188,7 @@
`llvm-prefer-isa-or-dyn-cast-in-conditionals `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-implementation-in-namespace `_,
`llvmlibc-restrict-system-libc-headers `_, "Yes"
`misc-definitions-in-headers `_, "Yes"
`misc-misplaced-const `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -113,6 +113,11 @@
   Flags use of the `C` standard library functions ``memset``, ``memcpy`` and
   ``memcmp`` and similar derivatives on non-trivial types.
 
+- New :doc:`llvmlibc-implementation-in-namespace
+  ` check.
+
+  Checks all llvm-libc implementation is within the correct namespace.
+
 - New :doc:`llvmlibc-restrict-system-libc-headers
   ` check.
 
Index: 

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:21
+  Finder->addMatcher(
+  decl(hasParent(translationUnitDecl()), unless(linkageSpecDecl()))
+  .bind("child_of_translation_unit"),

This skips linkage spec declarations, but are there other declarations that 
should be similarly skipped? For instance `static_assert` declarations?



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:33-34
+
+  if (isa(MatchedDecl)) {
+const auto *NS = cast(MatchedDecl);
+if (NS->getName() != RequiredNamespace) {

Instead of doing an `isa<>` followed by a `cast<>`, the more common pattern is 
to do:
```
if (const auto *NS = dyn_cast(MatchedDecl)) {
```



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:36
+if (NS->getName() != RequiredNamespace) {
+  diag(NS->getLocation(), "'%0' needs to be the outermost namespace.")
+  << RequiredNamespace;

clang-tidy diagnostics don't have punctuation, so you should drop the full stop.



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp:42
+  diag(MatchedDecl->getLocation(),
+   "Please wrap implentation in '%0' namespace.")
+  << RequiredNamespace;

They also aren't grammatically correct sentences, so the capital P and period 
should both go. While this definitely gets points for politeness, I think a 
more typical diagnostic might be: `declaration must be declared within the '%0' 
namespace`



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h:35
+private:
+  std::string RequiredNamespace;
+};

njames93 wrote:
> This can be made const
Will there only ever be a single namespace? Or should this be a list (for 
instance, a main namespace and a details namespace)?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst:6
+
+Checks all llvm-libc implementation is within the correct namespace.
+

Checks that all declarations in the llvm-libc implementation are within the 
correct namespace.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst:32-35
+.. option:: RequiredNamespace
+
+The namespace that llvm-libc implementations must be wrapped in. The 
default
+is `__llvm_libc`.

Given that this check is specific to llvm-libc, why is the option needed at all?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

This check needs a store Options override method as it reading options from a 
configuration. Check other checks to see how this is done.




Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h:35
+private:
+  std::string RequiredNamespace;
+};

This can be made const


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 254376.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818

Files:
  clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h
  clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
@@ -0,0 +1,40 @@
+// RUN: %check_clang_tidy %s llvmlibc-implementation-in-namespace %t
+
+#define MACRO_A "defining macros outside namespace is valid"
+
+class ClassB;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Please wrap implentation in '__llvm_libc' namespace.
+struct StructC {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: Please wrap implentation in '__llvm_libc' namespace.
+char *VarD = MACRO_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Please wrap implentation in '__llvm_libc' namespace.
+typedef int typeE;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Please wrap implentation in '__llvm_libc' namespace.
+void funcF() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Please wrap implentation in '__llvm_libc' namespace.
+
+namespace namespaceG {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: '__llvm_libc' needs to be the outermost namespace.
+namespace __llvm_libc{
+namespace namespaceH {
+class ClassB;
+} // namespace namespaceH
+struct StructC {};
+} // namespace __llvm_libc
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+} // namespace namespaceG
+
+// Wrapped in correct namespace.
+namespace __llvm_libc {
+// Namespaces within __llvim_libc namespace allowed.
+namespace namespaceI {
+class ClassB;
+} // namespace namespaceI
+struct StructC {};
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+extern "C" void extern_funcJ() {}
+} // namespace __llvm_libc
Index: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - llvmlibc-implementation-in-namespace
+
+llvmlibc-implementation-in-namespace
+
+
+Checks all llvm-libc implementation is within the correct namespace.
+
+.. code-block:: c++
+
+// Correct: implementation inside the correct namespace.
+namespace __llvm_libc {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+// Namespaces within __llvm_libc namespace are allowed.
+namespace inner{
+int localVar = 0;
+}
+// Functions with C linkage are allowed.
+extern "C" void str_fuzz(){}
+}
+
+// Incorrect: implementation not in a namespace.
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+
+// Incorrect: outer most namespace is not correct.
+namespace something_else {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+}
+
+Options
+---
+
+.. option:: RequiredNamespace
+
+The namespace that llvm-libc implementations must be wrapped in. The default
+is `__llvm_libc`.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -188,6 +188,7 @@
`llvm-prefer-isa-or-dyn-cast-in-conditionals `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-implementation-in-namespace `_,
`llvmlibc-restrict-system-libc-headers `_, "Yes"
`misc-definitions-in-headers `_, "Yes"
`misc-misplaced-const `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -113,6 +113,11 @@
   Flags use of the `C` standard library functions ``memset``, ``memcpy`` and
   ``memcmp`` and similar derivatives on non-trivial types.
 
+- New :doc:`llvmlibc-implementation-in-namespace
+  ` check.
+
+  Checks all llvm-libc implementation is within the correct namespace.
+
 - New :doc:`llvmlibc-restrict-system-libc-headers
   ` check.
 
Index: clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Siva Chandra via Phabricator via cfe-commits]

sivachandra added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp:16
+
+namespace namespaceG {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: '__llvm_libc' needs to be the 
outermost namespace.

Can you add `__llvm_libc` as a nested namespace here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast added a comment.

Friendly Ping @njames93 (:
Any other changes needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast marked 3 inline comments as done.
PaulkaToast added a comment.

In D76818#1943781 , @njames93 wrote:

> If you want to make it a general check, you should consider making the 
> default module options set the correct namespace
> RequiredNamespace


Changed the check a bit so I'm not sure if pulling it out is still something 
desired, if so what module do you recommend this should live under?




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp:6
+class ClassB;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: CXXRecord is not defined in a 
namespace, please wrap implentation in '__llvm_libc' namespace.
+struct StructC {};

njames93 wrote:
> Small nit : Not a fan of the diagnostic saying `CXX Record`. Maybe a pain but 
> `getDeclKindName` isn't the best to expose to users. 
Removed getDeclKindName as recommended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast updated this revision to Diff 253250.
PaulkaToast added a comment.

Updated to handle nested namespaces, exclude C linkages functions,  and made 
check language specific. (:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818

Files:
  clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h
  clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
@@ -0,0 +1,38 @@
+// RUN: %check_clang_tidy %s llvmlibc-implementation-in-namespace %t
+
+#define MACRO_A "defining macros outside namespace is valid"
+
+class ClassB;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Please wrap implentation in '__llvm_libc' namespace.
+struct StructC {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: Please wrap implentation in '__llvm_libc' namespace.
+char *VarD = MACRO_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Please wrap implentation in '__llvm_libc' namespace.
+typedef int typeE;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Please wrap implentation in '__llvm_libc' namespace.
+void funcF() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Please wrap implentation in '__llvm_libc' namespace.
+
+namespace namespaceG {
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: '__llvm_libc' needs to be the outermost namespace.
+namespace namespaceH {
+class ClassB;
+} // namespace namespaceH
+struct StructC {};
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+} // namespace namespaceG
+
+// Wrapped in correct namespace.
+namespace __llvm_libc {
+// Namespaces within __llvim_libc namespace allowed.
+namespace namespaceI {
+class ClassB;
+} // namespace namespaceI
+struct StructC {};
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+extern "C" void extern_funcJ() {}
+} // namespace __llvm_libc
Index: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - llvmlibc-implementation-in-namespace
+
+llvmlibc-implementation-in-namespace
+
+
+Checks all llvm-libc implementation is within the correct namespace.
+
+.. code-block:: c++
+
+// Correct: implementation inside the correct namespace.
+namespace __llvm_libc {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+// Namespaces within __llvm_libc namespace are allowed.
+namespace inner{
+int localVar = 0;
+}
+// Functions with C linkage are allowed.
+extern "C" void str_fuzz(){}
+}
+
+// Incorrect: implementation not in a namespace.
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+
+// Incorrect: outer most namespace is not correct.
+namespace something_else {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+}
+
+Options
+---
+
+.. option:: RequiredNamespace
+
+The namespace that llvm-libc implementations must be wrapped in. The default
+is `__llvm_libc`.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -188,6 +188,7 @@
`llvm-prefer-isa-or-dyn-cast-in-conditionals `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-implementation-in-namespace `_,
`llvmlibc-restrict-system-libc-headers `_, "Yes"
`misc-definitions-in-headers `_, "Yes"
`misc-misplaced-const `_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -113,6 +113,11 @@
   Flags use of the `C` standard library functions ``memset``, ``memcpy`` and
   ``memcmp`` and similar derivatives on non-trivial types.
 
+- New :doc:`llvmlibc-implementation-in-namespace
+  ` check.
+
+  Checks all llvm-libc implementation is within the correct namespace.
+
 - New :doc:`llvmlibc-restrict-system-libc-headers
   ` check.
 
Index: 

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

If you want to make it a general check, you should consider making the default 
module options set the correct namespace
RequiredNamespace

  ClangTidyOptions getModuleOptions() override {
ClangTidyOptions Options;

Options.CheckOptions["llvmlibc-implementation-in-namespace.RequiredNamespace"] 
= "__llvm_libc";
return Options;
  }




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp:6
+class ClassB;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: CXXRecord is not defined in a 
namespace, please wrap implentation in '__llvm_libc' namespace.
+struct StructC {};

Small nit : Not a fan of the diagnostic saying `CXX Record`. Maybe a pain but 
`getDeclKindName` isn't the best to expose to users. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Siva Chandra via Phabricator via cfe-commits]

sivachandra added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp:30
+// Wrapped in correct namespace.
+namespace __llvm_libc {
+class ClassB;

For completeness, can you include a nested namespace and a function declaration 
here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please also add isLanguageVersionSupported(), because namespaces make sense 
only in C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76818



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76818: [clang-tidy] Add check llvmlibc-implementation-in-namespace.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast created this revision.
PaulkaToast added reviewers: alexfh, aaron.ballman, hokein, njames93.
PaulkaToast added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, MaskRay, xazax.hun, mgorny.
Herald added a project: clang.

This check makes sure all llvm-libc implementations falls within the 
`__llvm_libc` namespace.

The check is quite general, lots of projects do this by conversion to avoid 
pollution. Not sure if there is a desire to put this in a different module?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76818

Files:
  clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/ImplementationInNamespaceCheck.h
  clang-tools-extra/clang-tidy/llvmlibc/LLVMLibcTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
  
clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-implementation-in-namespace.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s llvmlibc-implementation-in-namespace %t
+
+#define MACRO_A "defining macros outside namespace is valid"
+
+class ClassB;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: CXXRecord is not defined in a namespace, please wrap implentation in '__llvm_libc' namespace.
+struct StructC {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: CXXRecord is not defined in a namespace, please wrap implentation in '__llvm_libc' namespace.
+char *VarD = MACRO_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Var is not defined in a namespace, please wrap implentation in '__llvm_libc' namespace.
+typedef int typeE;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Typedef is not defined in a namespace, please wrap implentation in '__llvm_libc' namespace.
+void funcF() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Function is not defined in a namespace, please wrap implentation in '__llvm_libc' namespace.
+
+namespace namespace_g {
+class ClassB;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: CXXRecord is defined in namespace 'namespace_g', should be in '__llvm_libc' namespace.
+struct StructC {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: CXXRecord is defined in namespace 'namespace_g', should be in '__llvm_libc' namespace.
+char *VarD = MACRO_A;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: Var is defined in namespace 'namespace_g', should be in '__llvm_libc' namespace.
+typedef int typeE;
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: Typedef is defined in namespace 'namespace_g', should be in '__llvm_libc' namespace.
+void funcF() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Function is defined in namespace 'namespace_g', should be in '__llvm_libc' namespace.
+} // namespace namespace_g
+
+// Wrapped in correct namespace.
+namespace __llvm_libc {
+class ClassB;
+struct StructC {};
+char *VarD = MACRO_A;
+typedef int typeE;
+void funcF() {}
+} // namespace __llvm_libc
Index: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvmlibc-implementation-in-namespace.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - llvmlibc-implementation-in-namespace
+
+llvmlibc-implementation-in-namespace
+
+
+Checks all llvm-libc implementation is within the correct namespace.
+
+.. code-block:: c++
+
+// Correct: implementation inside the correct namespace.
+namespace __llvm_libc {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+}
+
+// Incorrect: implementation not in a namespace.
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+
+// Incorrect: implementation inside an incorrect namespace.
+namespace something_else {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+}
+
+Options
+---
+
+.. option:: RequiredNamespace
+
+The namespace that llvm-libc implementations must be wrapped in. The default
+is `__llvm_libc`.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -188,6 +188,7 @@
`llvm-prefer-isa-or-dyn-cast-in-conditionals `_, "Yes"
`llvm-prefer-register-over-unsigned `_, "Yes"
`llvm-twine-local `_, "Yes"
+   `llvmlibc-implementation-in-namespace `_,
`llvmlibc-restrict-system-libc-headers `_, "Yes"
`misc-definitions-in-headers `_, "Yes"