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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

The TableGen part LGTM. I have left couple of comments about the clang-tidy part.

I have a higher level question. Given our rule for include files, the check performed by the qualified call checker seems redundant to me. Am I right, or am I missing something?



================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:77
+    // If the call is prefixed with the qualified __llvm_libc:: it's good to go.
+    if (llvm::StringRef(funcDecl->getQualifiedNameAsString())
+            .startswith("__llvm_libc::"))
----------------
I think they way it is written is actually what we want. But, it took me a long time to figure out why it was working at all because of the way you wrote your test. I put in some explanation over there about what is not expected in the test.

So, the way this check is written, if it finds a call-site to a public function provided by LLVM-libc, it checks whether it resolves to the LLVM-libc declaration. I think we want the check to be just this. We do not need that the call-site actually uses the fully qualified name. As long as we ensure the call-site resolves to an LLVM-libc function, C++ name mangling will give us enough confidence that it is not getting mixed up with a function with the same function from another libc.


================
Comment at: libc/utils/LibcTidy/tests/Test.cpp:20
+  // Disallow just plain calls.
+  libc_api_func();
+  // CHECK: :[[@LINE-1]]:3: error: libc_api_func must be called with qualified name '__llvm_libc'.
----------------
This error is confusing. At least, it confused me. IIUC, an error will be reported even if the call were to be made in this fashion:

```
::libc_api_func();
```

>From the user's point of view, this is total fine: they are explicitly calling the global version so there should be no error. Even if we had a non-global namespace like this, an error will be reported:

```
namespace nn {
void libc_api_func() {}
}
...
nn::libc_api_func();
```

Such uses cases are relevant in tests and I think we should allow them in tests. Also, did you intend to allow a use case like this:

```
namespace __llvm_libc {
void some_func() {
  libc_api_func(); // Unqualified call to libc_api_func.
}
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77281