[PATCH] D113946: [libc][clang-tidy] fix namespace check for externals

[Siva Chandra via Phabricator via cfe-commits]

sivachandra added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp:1
 //===-- CalleeNamespaceCheck.cpp ------------------------------------------===//
 //
----------------
You should add tests for this exception check. See https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/llvmlibc-callee-namespace.cpp.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp:13
 
+#include <stdio.h>
+
----------------
Eugene.Zelenko wrote:
> Should  be `<cstdio>` and without newline separation form rest of headers.
Looks like this is present only for the debug `printf`?


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp:39
+// intercepted.
+static const char *FUNCTIONS_TO_IGNORE_NAMESPACE[] = {
+    "__errno_location", "malloc", "calloc", "realloc", "free"};
----------------
Eugene.Zelenko wrote:
> Why not `std::array` of appropriate LLVM container?
May be `static const std::uordered_set<llvm::StringRef>`? It would likely make the lookup below much neater.


================
Comment at: clang-tools-extra/clang-tidy/llvmlibc/CalleeNamespaceCheck.cpp:59
+            llvm::StringRef(FUNCTIONS_TO_IGNORE_NAMESPACE[i]))) {
+      printf("String found %s\n", FuncDecl->getName().str().c_str());
+      return;
----------------
lntue wrote:
> Look like diag() is used to print messages in this module?
This looks like a debug `printf`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113946