This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Refactor clang-query checks to clang-tidy checks to get less obscure error messages
ClosedPublic

Authored by philnik on Jan 15 2023, 3:05 PM.

Details

Summary

Also remove clang-query related code, since it's unused now.

Diff Detail

Event Timeline

philnik created this revision.Jan 15 2023, 3:05 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: arichardson. · View Herald Transcript
philnik requested review of this revision.Jan 15 2023, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2023, 3:05 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I like the removal of a dependency, especially since clang-tidy shows better diagnostics.
Some questions and remarks.

libcxx/docs/TestingLibcxx.rst
101

Can we remove clang-query from the Dockerfile too or is it still needed there?

101

I think there are more dependencies for clang-tidy with the plugins, is that correct?

libcxx/include/__memory_resource/monotonic_buffer_resource.h
83 ↗(On Diff #489398)

Is this intended in this patch? It feels completely unrelated to test refactoring.
If it's not related can you move this to a new review and let this one depend on it?

libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
8–10

Not entirely related to this change, but is there any documentation describing how to use this locally.
(I tried in the past and failed, I expect I did something wrong, but couldn't figure out what quickly.)

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
37

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:: ?

philnik updated this revision to Diff 489597.Jan 16 2023, 10:31 AM
philnik marked 2 inline comments as done.

Address comments

libcxx/docs/TestingLibcxx.rst
101

IIRC clang-query part of the clang-tools package, which I think we still depend on. I might be wrong about that though.

101

Kind-of, depends on how your OS packages things. If the headers and object files are included within the same package, there are no additional dependencies. Added a comment.

libcxx/include/__memory_resource/monotonic_buffer_resource.h
83 ↗(On Diff #489398)
libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
8–10

No. I can write some short documentation, but I don't know how much help this will actually be, since this can be very environment-specific. I guess we could expand this with problems people had when setting it up, so others have an easier time.

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
37

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.

Mordante added inline comments.Jan 17 2023, 11:53 AM
libcxx/docs/TestingLibcxx.rst
101

It's indeed part of clang-tools and I think there's nothing there we use:
https://packages.debian.org/bullseye/amd64/clang-tools-11/filelist

libcxx/test/tools/clang_tidy_checks/CMakeLists.txt
8–10

If you write an initial version I'll test it on Debian and update if needed. I want to use this myself to add some additional checks; also to get a bit of experience with writing my own checks.

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
37

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

philnik updated this revision to Diff 489923.Jan 17 2023, 12:52 PM
philnik marked 4 inline comments as done.

Address comments

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
37

It would also be ignored: https://godbolt.org/z/YPdY6WqP7

njames93 added inline comments.Jan 17 2023, 2:45 PM
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
25–35

Can you not use the hasAnyName matcher here instead?

philnik updated this revision to Diff 489953.Jan 17 2023, 3:07 PM
philnik marked an inline comment as done.

Address comments

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
25–35

Thanks, I wasn't aware of that matcher.

Mordante retitled this revision from [libc++] Refactor clang-query checks to clang-tidy checks to get less obsucre error messages to [libc++] Refactor clang-query checks to clang-tidy checks to get less obscure error messages.Jan 18 2023, 9:45 AM
Mordante added inline comments.Jan 18 2023, 9:52 AM
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
37

Then I think it would be better to test against the fully qualified name.

philnik marked an inline comment as done.Jan 18 2023, 2:08 PM
philnik added inline comments.
libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
37

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.

ldionne accepted this revision as: ldionne.Jan 18 2023, 2:12 PM

I like this!

Mordante accepted this revision.Jan 22 2023, 4:50 AM

LGTM modulo one comment.

libcxx/test/tools/clang_tidy_checks/hide_from_abi.cpp
37

Can you add that information to the comment. I still prefer FQN, but if that's hard let's document it.

This revision is now accepted and ready to land.Jan 22 2023, 4:50 AM
This revision was landed with ongoing or failed builds.Jan 22 2023, 8:01 PM
This revision was automatically updated to reflect the committed changes.
philnik marked 4 inline comments as done.
libcxx/test/libcxx/clang_query.sh.cpp