[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);
----------------
Is it a large change to worry about global variables?


================
Comment at: libc/utils/LibcTidy/CalleeNamespaceCheck.h:39
+    // Ignore special unnamed C++ functions like contructors or operators.
+    if (funcDecl->getIdentifier() == NULL)
+      return;
----------------
Maybe `!funcDecl->...` makes more sense? But if not this should at least be `nullptr`


================
Comment at: libc/utils/LibcTidy/CalleeNamespaceCheck.h:43
+    // We check the start of the fully qualified name of the function decl.
+    if (llvm::StringRef(funcDecl->getQualifiedNameAsString())
+            .startswith("__llvm_libc::"))
----------------
>From looking at the ast dump there is a reference to the declaration being called. I'm sure there is a way to see if that declaration was namespaced or not. But I wonder if we anyway want to require we use the full namespaced name?


================
Comment at: libc/utils/LibcTidy/EntrypointNameCheck.h:25-26
+
+  void MacroExpands(const Token &macroNameTok, const MacroDefinition &MD,
+                    SourceRange range, const MacroArgs *args) override {
+    if (macroNameTok.getIdentifierInfo()->getName() != "LLVM_LIBC_ENTRYPOINT")
----------------
We can omit the `MD` and `range` names to avoid any unused variable warnings


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:83
+    DiagnosticsEngine &DE = result.Context->getDiagnostics();
+    const unsigned ID = DE.getCustomDiagID(
+        DiagnosticsEngine::Error,
----------------
PaulkaToast wrote:
> abrachet wrote:
> > No point for `const`
> Ah, sorry I might be missing something. What is the reasoning for not using const here? I thought it's good practice to use const anytime you don't plan to modify the variable? To prevent mistakes like `ID += 1;` as this compiles but yields runtime errors. 
It would prevent that, but I think it could become verbose. `const` is useful to ensure a contract between functions not as much within one. It's a stylistic choice I don't have a large preference.


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:107
+  std::string programName(argv[0]);
+  TableGenMain(&programName[0], &TidyTableGenMain);
+
----------------
This might work because `std::string::operator[]` returns `CharT &` but I think it's confusing. Probably `programName.data()` is easier. `TableGenMain` shouldn't require a `char *` though it should be `const char *`.


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:112
+  // Checks
+  std::vector<std::shared_ptr<LibcTidyCheck>> checks;
+
----------------
Why are these `shared_ptr` and not `unique_ptr`?

We could also initialize like `std::vector<...> checks {std::make_shared<CalleeNamespaceCheck>(), ... };` instead of calling push_back twice.


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.h:16
+
+bool isNOLINTPresent(const clang::SourceManager *SM,
+                     clang::SourceLocation loc) {
----------------
Do we care about a nolint on the previous line? Is that already handled by clang-tidy?


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.h:32-33
+public:
+  virtual void registerPPCallbacks(clang::Preprocessor &PP){};
+  virtual void registerMatchers(clang::ast_matchers::MatchFinder &finder){};
+};
----------------
Do we want these to be pure virtual or is there a reason they do nothing?


================
Comment at: libc/utils/LibcTidy/TableGenUtil.h:17
+
+using NameSet = std::unordered_set<std::string>;
+
----------------
FWIW, there exists an `llvm::StringSet`. Why would you use it over this? No clue.


================
Comment at: libc/utils/LibcTidy/tests/.clang-format:1
+BasedOnStyle: LLVM
+ColumnLimit: 0
----------------
I think `clang-format` keeps looking in `..` until it finds a `.clang-format` I might be wrong though. In any case is it needed to have a `.clang-format` file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77281