[libcxx-commits] [PATCH] D141805: [libc++] Refactor clang-query checks to clang-tidy checks to get less obscure error messages

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM modulo one comment.



================
Comment at: libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp:36
+              hasAttr(clang::attr::AbiTag),
+              cxxMethodDecl(), // We have explicitly instantiated classes and some of their methods don't have these attributes
+              isDeleted(),
----------------
philnik wrote:
> Mordante wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > philnik wrote:
> > > > > Mordante wrote:
> > > > > > I assume this has to to with the list of `hasName` above. If so let's move the comment there and please elaborate a bit more why these functions are excluded. Just to make sure that it's clear why this is needed.
> > > > > > 
> > > > > > A question, is `std::ranges::__stable_sort` also added to this list? Or only names in `std::` ?
> > > > > I'm, not sure what you are referring to with `I assume this has to do with the list of hasName above`.  The functions are excluded because they can't be marked `[[gnu::always_inline]]` for various reasons.
> > > > > 
> > > > > This check only looks through functions, so `ranges::__stable_sort` isn't related.
> > > > > 
> > > > Can you then add a comment like `These functions are excluded because they can't be marked [[gnu::always_inline]].`
> > > > What happens when `std::ranges::__stable_sort` is a function? I forgot it was a cpo
> > > It would also be ignored: https://godbolt.org/z/YPdY6WqP7
> > Then I think it would be better to test against the fully qualified name.
> I don't. Testing against the fully qualified name is a lot harder AFAICT, and we don't have functions with the same name in different namespaces normally, especially the ones listed here. The proper solution would be to hide them from the ABI without having `[[gnu::always_inline]]` on them. Then we wouldn't have to ignore the functions here and we don't inline everything with GCC.
Can you add that information to the comment. I still prefer FQN, but if that's hard let's document it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141805