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

[Paula Toth via Phabricator via libc-commits]

PaulkaToast added inline comments.


================
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'.
----------------
sivachandra wrote:
> sivachandra wrote:
> > 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.
> > }
> > }
> > ```
> I think this test is mostly correct. Few minor changes:
> 1. Enclose `Test` in the namespace `__llvm_libc`. 
> 2. The call at line 20 should not result in an error after that.
> 3. Add a call to `::libc_api_func` which should result in an error.
> 4. The error message in general should say something like, `error: call to {function} not declared in __llvm_libc namespace.` You can additionally point to the file:line of the function declaration to which the erroneous call resolves to.
Added some clarifying comments and tweaked the error message so it should make a big more sense now.


================
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'.
----------------
PaulkaToast wrote:
> sivachandra wrote:
> > sivachandra wrote:
> > > 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.
> > > }
> > > }
> > > ```
> > I think this test is mostly correct. Few minor changes:
> > 1. Enclose `Test` in the namespace `__llvm_libc`. 
> > 2. The call at line 20 should not result in an error after that.
> > 3. Add a call to `::libc_api_func` which should result in an error.
> > 4. The error message in general should say something like, `error: call to {function} not declared in __llvm_libc namespace.` You can additionally point to the file:line of the function declaration to which the erroneous call resolves to.
> Added some clarifying comments and tweaked the error message so it should make a big more sense now.
1. Done.
2. Yes, this is now valid and doesn't produce an error.
3. Done.
4. Modified error message and now it also points to the function that the call resolves to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77281