[PATCH] D76744: [clang-tidy] Add check to ensure llvm-libc implementations are defined in correct namespace.

[Paula Toth via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/EntrypointNamespaceCheck.cpp:67
+  if (Result.SourceManager->getFilename(MatchedDecl->getLocation())
+  .endswith(".h"))
+return;

njames93 wrote:
> Is there a rule that all libc implementation headers must have the extension 
> `.h`. If not there is `utils::FileExtensionSet` that could be used.
> Alternatively you could just check to see if the SourceLocation is in the 
> main file
> `if (!Result.SourceManager->isInMainFile(MatchedDecl->getLocation())`
Thanks for this! I incorporated your recommendations in the new patch. (:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76744



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

[PATCH] D76744: [clang-tidy] Add check to ensure llvm-libc implementations are defined in correct namespace.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast abandoned this revision.
PaulkaToast added a comment.

Abandoning for a more generalized check. D76818 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76744



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

[PATCH] D76744: [clang-tidy] Add check to ensure llvm-libc implementations are defined in correct namespace.

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

This check should only worry about the first declaration of the function, any 
redecls or the definition could appear outside the namespace like this:

  namespace blah{
  void foo();
  }
  void blah::foo();
  void blah::foo(){}

There are some missing test cases I'd like to see like
wrapping in an enclosing namespace (including anonymous)

  namespace __llvm_libc {
  namespace  {
  // impl
  }
  }
  
  namespace  {
  namesapce __llvm_libc {
  // impl
  }
  }

As I don't work on libc I'm not sure how these should be handled, maybe its 
fine if there is a corresponding `inline namespace`.




Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/EntrypointNamespaceCheck.cpp:67
+  if (Result.SourceManager->getFilename(MatchedDecl->getLocation())
+  .endswith(".h"))
+return;

Is there a rule that all libc implementation headers must have the extension 
`.h`. If not there is `utils::FileExtensionSet` that could be used.
Alternatively you could just check to see if the SourceLocation is in the main 
file
`if (!Result.SourceManager->isInMainFile(MatchedDecl->getLocation())`



Comment at: 
clang-tools-extra/clang-tidy/llvmlibc/EntrypointNamespaceCheck.cpp:71
+  std::string RequiredNamespace =
+  Options.get("RequiredNamespace", "__llvm_libc");
+  const DeclContext *EnclosingDecl =

To save fetching the option each time, why not just store the required 
namespace in the class and initialize it in the constructor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76744



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

[PATCH] D76744: [clang-tidy] Add check to ensure llvm-libc implementations are defined in correct namespace.

[Siva Chandra via Phabricator via cfe-commits]

sivachandra added a comment.

Instead of this narrow check, what we really want is a check to ensure that all 
implementation detail resides in the namespace `__llvm_libc`. Anything outside 
would be special and requiring a `NOLINT...` for them is reasonable. Have you 
considered such an approach?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76744



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

[PATCH] D76744: [clang-tidy] Add check to ensure llvm-libc implementations are defined in correct namespace.

[Paula Toth via Phabricator via cfe-commits]

PaulkaToast created this revision.
PaulkaToast added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, MaskRay, xazax.hun, mgorny.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76744

Files:
  clang-tools-extra/clang-tidy/llvmlibc/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvmlibc/EntrypointNamespaceCheck.cpp
  clang-tools-extra/clang-tidy/llvmlibc/EntrypointNamespaceCheck.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-entrypoint-namespace.rst
  clang-tools-extra/test/clang-tidy/checkers/llvmlibc-entrypoint-namespace.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/llvmlibc-entrypoint-namespace.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/llvmlibc-entrypoint-namespace.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s llvmlibc-entrypoint-namespace %t
+
+#define LLVM_LIBC_ENTRYPOINT(name) name
+#define SOMETHING_ELSE(name) name
+
+// use of entrypoint macro with correct namespace.
+namespace __llvm_libc {
+void LLVM_LIBC_ENTRYPOINT(correct_entrypoint)(char *param) {}
+} // namespace __llvm_libc
+
+// different macros are ignored.
+namespace something_else {
+void SOMETHING_ELSE(different_macro)(char *param) {}
+} // namespace something_else
+
+// Not in a namespace.
+void LLVM_LIBC_ENTRYPOINT(missing_namespace)(char *param) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: function 'missing_namespace' is not defined in a namespace, please wrap implentation in '__llvm_libc' namespace.
+
+// Inside incorrect namespace.
+namespace something_else {
+void LLVM_LIBC_ENTRYPOINT(incorrect_namespace)(char *param) {}
+} // namespace something_else
+// CHECK-MESSAGES: :[[@LINE-2]]:27: warning: function 'incorrect_namespace' is defined in namespace 'something_else', should be in '__llvm_libc' namespace.
Index: clang-tools-extra/docs/clang-tidy/checks/llvmlibc-entrypoint-namespace.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvmlibc-entrypoint-namespace.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - llvmlibc-entrypoint-namespace
+
+llvmlibc-entrypoint-namespace
+=
+
+Finds where llvm-libc entrypoint macro is called and checks that it is wrapped
+in the correct namespace.
+
+.. code-block:: c++
+
+// Correct: entrypoint inside correct namespace.
+namespace __llvm_libc {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+}
+
+// Incorrect: entrypoint not in a namespace.
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+
+// Incorrect: entrypoint inside incorrect namespace.
+namespace something_else {
+void LLVM_LIBC_ENTRYPOINT(strcpy)(char *dest, const char *src) {}
+}
+
+Options
+---
+
+.. option:: EntrypointMacro
+
+The name of the macro used when defining llvm-libc implementations. The
+default is `LLVM_LIBC_ENTRYPOINT`.
+
+.. 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-entrypoint-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
@@ -92,7 +92,7 @@
 
   Finds ``cnd_wait``, ``cnd_timedwait``, ``wait``, ``wait_for``, or
   ``wait_until`` function calls when the function is not invoked from a loop
-  that checks whether a condition predicate holds or the function has a 
+  that checks whether a condition predicate holds or the function has a
   condition parameter.
 
 - New :doc:`bugprone-reserved-identifier
@@ -113,6 +113,12 @@
   Flags use of the `C` standard library functions ``memset``, ``memcpy`` and
   ``memcmp`` and similar derivatives on non-trivial types.
 
+- New :doc:`llvmlibc-entrypoint-namespace
+  ` check.
+
+  Finds where llvm-libc entrypoint macro is called and checks that it is wrapped
+  in the correct namespace.
+
 - New :doc:`llvmlibc-restrict-system-libc-headers
   ` check.
 
@@ -156,7 +162,7 @@
 ^^
 
 - Improved :doc:`readability-qualified-auto
-  ` check now supports a 
+  `