[libc-commits] [PATCH] D77281: [libc] Add libc-tidy.

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/utils/LibcTidy/CalleeNamespaceCheck.h:21
+    StatementMatcher qualifiedCallMatcher =
+        callExpr(callee(functionDecl().bind("func"))).bind("call-site");
+    finder.addMatcher(qualifiedCallMatcher, this);
----------------
sivachandra wrote:
> PaulkaToast wrote:
> > abrachet wrote:
> > > Is it a large change to worry about global variables?
> > Do you mean global variables that might not be in the namespace?
> Hmm, may be? Should we wait until we have an example which causes a problem?
I think `stdout` and friends would be worth having this for but we don't have them yet. Waiting is perfectly fine.


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:58
+  CreateASTConsumer(CompilerInstance &compiler,
+                    llvm::StringRef inFile) override {
+    Preprocessor &PP = compiler.getPreprocessor();
----------------
Remove `inFile`


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.h:13
+#include "clang/Basic/Diagnostic.h"
+#include "llvm/ADT/StringSet.h"
+
----------------
Looks like this include isn't needed either


================
Comment at: libc/utils/LibcTidy/TableGenUtil.cpp:22-25
+    auto functionNameList = def->getValueAsListOfStrings("Functions");
+    for (llvm::StringRef functionName : functionNameList) {
+      funcNames.insert(functionName);
+    }
----------------
`funcNames.insert(functionNameList.begin(), functionNameList.end())`


================
Comment at: libc/utils/LibcTidy/tests/CalleeNamespaceTest.cpp:22-24
+  // Should not trigger on system provided function calls.
+  va_list ap;
+  va_end(ap);
----------------
FWIW, `va_*` functions almost certainly are macros which expand to `__builtin_va_*`. I'm not sure there exist any system provided function calls (which have definitions in public headers that is). 


================
Comment at: libc/utils/LibcTidy/tests/mock_api.td:8-12
+  let TypeDeclarations = [
+  ];
+
+  let Macros = [
+  ];
----------------
I don't think its required to have these empty they can be omitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77281