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

Fri Apr 24 16:16:52 PDT 2020

PaulkaToast added inline comments.

================
Comment at: libc/utils/LibcTidy/CalleeNamespaceCheck.h:39
+    // Ignore special unnamed C++ functions like contructors or operators.
+    if (funcDecl->getIdentifier() == NULL)
+      return;
----------------
sivachandra wrote:
> PaulkaToast wrote:
> > 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 (:
> AFAIU, constructors and operators have fully qualified name and the `getQualifiedNameAsString` should give you the name. Do you have an example where this is breaking?
Clang doesn't provide the qualified name as a string for these types of special functions. https://clang.llvm.org/doxygen/classclang_1_1NamedDecl.html#a7fab32caa3b7d601128265248260fd37

See this error produced:
```
libc-tidy: workspace/llvm-project/llvm/../clang/include/clang/AST/Decl.h:251: llvm::StringRef clang::NamedDecl::getName() const: Assertion `Name.isIdentifier() && "Name is not a simple identifier"' failed.
```
on this line:
https://github.com/llvm/llvm-project/blob/60f1d2636626a0b92b6ad2cb86eea78cb1cd7b33/libc/src/signal/linux/signal.h#L33
and
https://github.com/llvm/llvm-project/blob/4fd92cc475567f2001344d7049fe7ac592e8bdbe/libc/utils/testutils/StreamWrapper.h#L24
for example.

================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:103-105
+  // TODO: argv needs to be const for tooling's OptionParser above but
+  // TableGenMain expects a regular char*. There might be a cleaner way
+  // to grab argv[0] as a plain char* from the const version above...
----------------
abrachet wrote:
> You can pass non const to const. So making `argv` just `char **` and then we don't need to copy into the string should work I believe.
Yeah that's what I thought too for passing `char *` to something that expects `const char *` but this results in this error ):
```
workspace/llvm-project/libc/utils/LibcTidy/LibcTidyTool.cpp:95:32: error: no matching constructor for initialization of 'tooling::CommonOptionsParser'
  tooling::CommonOptionsParser OptionsParser(argc, argv, LibCTidyCategory);
                               ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
workspace/llvm-project/llvm/../clang/include/clang/Tooling/CommonOptionsParser.h:75:3: note: candidate constructor not viable: no known conversion from 'char **' to 'const char **' for 2nd argument
  CommonOptionsParser(int &argc, const char **argv,
```

================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:116
+
+  for (auto check : checks)
+    check->registerMatchers(finder);
----------------
abrachet wrote:
> `auto &` wether we can use `unique_ptr` or not.
Okay, I think I was using unique_ptr incorrectly by not passing the vector as a reference in some places, although now we can't use the initializer pattern because under the hood it makes a copy. https://stackoverflow.com/questions/9618268/initializing-container-of-unique-ptrs-from-initializer-list-fails-with-gcc-4-7

================
Comment at: libc/utils/LibcTidy/LibcTidyTool.h:16
+
+bool isNOLINTPresent(const clang::SourceManager *SM,
+                     clang::SourceLocation loc) {
----------------
sivachandra wrote:
> PaulkaToast wrote:
> > 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.
> Same question as @abrachet: Doesn't clang-tidy already do this? We should avoid writing custom C++ source parsers unless existing clang or other APIs do not serve our purpose.
clang-tidy currently doesn't seem to be written in a way that we can expand on it in our source tree similar to lib tooling. Their nolint code is marked static and only visible here: https://github.com/llvm/llvm-project/blob/cb1ee34e9d32fce84613827693a8ed3aff1d36cf/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L270

Their diagnostics consumer is deeply linked to the rest of clang-tidy so we can't pull out the parts we need. 
This could be possible if they exposed `isNOLINTFound()`in a header file.

================
Comment at: libc/utils/LibcTidy/tests/CalleeNamespaceTest.cpp:33
+  // Allow when NOLINT is explicitly requested.
+  libc_api_func(); // LIBC-NOLINT
+  // CHECK-NOT: error: Function libc_api_func must call to internal implementation in `__llvm_libc` namespace.
----------------
sivachandra wrote:
> Why do you need NOLINT on this? This should resolve to `__llvm_libc::libc_api_func`, no?
I meant to pre-fix this with `::` thanks for catching this.

================
Comment at: libc/utils/LibcTidy/tests/EntrypointNameTest.cpp:18
+// Entrypoint is invalid if the function is not a public libc function.
+void LLVM_LIBC_ENTRYPOINT(non_api_func)(char *param) {}
+// CHECK: :[[@LINE-1]]:27: error: Entrypoint non_api_func is not a public libc function.
----------------
sivachandra wrote:
> This is good. But, what we really want additionally is couple more things:
> 
> 1. The name listed in the build rule matches the name of the entrypoint function in the sources.
> 2. That there is an implementation of the entrypoint function in the sources, and that it is listed with `LLVM_LIBC_ENTRYPOINT` macro.
> 
> I think #1 requires that we add a command line arg to libc-tidy?
I have plans for this,  but it requires changes to the build system. I think this patch is already quite large. Could we try expand on this by adding more checks in another patch after getting these initial checks landed?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77281