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

[Siva Chandra via Phabricator via libc-commits]

sivachandra marked 2 inline comments as done.
sivachandra 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);
----------------
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?


================
Comment at: libc/utils/LibcTidy/CalleeNamespaceCheck.h:39
+    // Ignore special unnamed C++ functions like contructors or operators.
+    if (funcDecl->getIdentifier() == NULL)
+      return;
----------------
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?


================
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::"))
----------------
PaulkaToast wrote:
> 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. 
The high level point here is that we are not doing these checks as matter of style enforcement. We are doing this to ensure that the code we write does not accidentally resolve to functions from another libc. The style requirement is probably relevant, but feels overbearing at this point. If we find a case where lack of style is causing a problem, we can revisit this.

Keep in mind that this does not close the door for reviewers requesting to qualify calls to improve readability.


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:50
+
+class LibcTidyAction : public ASTFrontendAction {
+public:
----------------
To improve navigation, you should put this class in a different file and call this file something like `Main.cpp`.


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.h:16
+
+bool isNOLINTPresent(const clang::SourceManager *SM,
+                     clang::SourceLocation loc) {
----------------
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.


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.h:15
+
+#include "TableGenUtil.h"
+
----------------
Not needed?


================
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.
----------------
Why do you need NOLINT on this? This should resolve to `__llvm_libc::libc_api_func`, no?


================
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.
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77281