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

Thu Apr 23 23:18:48 PDT 2020

PaulkaToast 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);
----------------
abrachet wrote:
> Is it a large change to worry about global variables?
Do you mean global variables that might not be in the namespace?

================
Comment at: libc/utils/LibcTidy/CalleeNamespaceCheck.h:39
+    // Ignore special unnamed C++ functions like contructors or operators.
+    if (funcDecl->getIdentifier() == NULL)
+      return;
----------------
abrachet wrote:
> Maybe `!funcDecl->...` makes more sense? But if not this should at least be `nullptr`
Ah yes thank you for catching this, not good practice on my part (:

================
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::"))
----------------
abrachet wrote:
> 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?
That was my original idea, however @sivachandra mentioned that he doesn't want to enforce a style such as requiring the call to be fully qualified and that if it resolves to the correct implementation that is sufficient.  The benefit, from what I understand is that this allows for a more general c++ looking code since its not very common to call qualified names from within a namespace but I'm sure Siva can speak more about the rationale. 

================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:72
+
+    // Only look for libc public API function calls.
+    if (functions.find(funcDecl->getNameInfo().getAsString()) ==
----------------
sivachandra wrote:
> Should we restrict to only entrypoints? Seems to me like we should do this for all function calls? That is, LLVM libc implementations (not redirectors) should only call functions declared in the namespace `__llvm_libc`.
This new implementation does this for all function calls, ignoring cases where the resolving function is system or compiler provided.

================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:107
+  std::string programName(argv[0]);
+  TableGenMain(&programName[0], &TidyTableGenMain);
+
----------------
abrachet wrote:
> 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 *`.
Tried this out and it complains about `.data()` returning a const pointer. `.data()` returning a char * is a C++17 feature and seems like LLVM is using C++14 https://llvm.org/docs/CodingStandards.html#c-standard-versions

================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:112
+  // Checks
+  std::vector<std::shared_ptr<LibcTidyCheck>> checks;
+
----------------
abrachet wrote:
> 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.
AFAIK they can't be `unique_ptr` because the checks lifetime need to extend until the end of `Tool.run()`, but we'd have to hand over ownership when calling `check-registerPPCallbacks()` this is why I went with the `shared_ptr` instead. I'm not sure if this is the best approach but I'm open to suggestions.

Also thank you this is a great pattern and eliminates the `push_back()`. (:

================
Comment at: libc/utils/LibcTidy/LibcTidyTool.h:16
+
+bool isNOLINTPresent(const clang::SourceManager *SM,
+                     clang::SourceLocation loc) {
----------------
abrachet wrote:
> Do we care about a nolint on the previous line? Is that already handled by clang-tidy?
This is intended to be quite lightweight so I didn't bother implementing a previous line nolint. I figured that using no-lint is a rare case. However if in the future this becomes cumbersome we could possibly add it in later.

================
Comment at: libc/utils/LibcTidy/LibcTidyTool.h:32-33
+public:
+  virtual void registerPPCallbacks(clang::Preprocessor &PP){};
+  virtual void registerMatchers(clang::ast_matchers::MatchFinder &finder){};
+};
----------------
abrachet wrote:
> Do we want these to be pure virtual or is there a reason they do nothing?
The `registerMatchers` might be a good candidate for a pure virtual function. However, `registerPPCallbacks` is only needed for cases where we need information from the pre-processor, so having an empty implementation there is probably better. In this patch we even have an example of a check that doesn't need a `registerMatchers` function because we register the AST matchers in the preprocessor callback. That was the motivation for making them do nothing by default.

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

================
Comment at: libc/utils/LibcTidy/tests/.clang-format:1
+BasedOnStyle: LLVM
+ColumnLimit: 0
----------------
abrachet wrote:
> 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?
Yes, unfortunately we have to tell clang-format to not break over long lines. This is because in FileCheck, the following lines are semantically different. 

```
// CHECK: :[[@LINE-1]]:3: error: Function libc_api_func must call to internal implementation in `__llvm_libc` namespace.
```
and 
```
// CHECK: :[[@LINE-1]]:3: error: Function libc_api_func must call to internal
// implementation in `__llvm_libc` namespace.
```
see also:  https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/.clang-format

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77281