Page MenuHomePhabricator

[clang][Tooling] Move STL recognizer to its own library
ClosedPublic

Authored by kadircet on Oct 5 2022, 12:54 AM.

Details

Summary

As pointed out in https://reviews.llvm.org/D119130#3829816, this
introduces a clang AST dependency to the clangToolingInclusions, which is used
by clang-format.

Since rest of the inclusion tooling doesn't depend on clang ast, moving this
into a separate library.

Diff Detail

Event Timeline

kadircet created this revision.Oct 5 2022, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 12:54 AM
Herald added a subscriber: arphaman. · View Herald Transcript
kadircet requested review of this revision.Oct 5 2022, 12:54 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 5 2022, 12:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sammccall accepted this revision.Oct 5 2022, 3:34 AM
sammccall added inline comments.
clang-tools-extra/clangd/CMakeLists.txt
163

StandardLibrary or Stdlib?

STL isn't accurate or consistent with the names in the code.

clang/lib/Tooling/Inclusions/STL/CMakeLists.txt
2 ↗(On Diff #465306)

This means the implementation files and the header files have a different directory structure, which may be confusing to people trying to work out which library to link against based on the headers they included.

On the other hand, I think the cascading effect of dependencies => libraries => directory structure => header structure is pretty unfortunate leaking of llvm's sad cmake structure. Up to you

This revision is now accepted and ready to land.Oct 5 2022, 3:34 AM
kadircet marked 2 inline comments as done.Oct 6 2022, 12:51 AM
kadircet added inline comments.
clang-tools-extra/clangd/CMakeLists.txt
163

changing to Stdlib

clang/lib/Tooling/Inclusions/STL/CMakeLists.txt
2 ↗(On Diff #465306)

right, i was also torn between moving the headers around vs not. but i finally managed to convince myself that the implementation being in a different subdirectory is actually an unfortunate detail about the way LLVM is build (I didn't want to have PARTIAL_SOURCES_INTENDED, either) and shouldn't matter for the applications that want to use it.

kadircet updated this revision to Diff 465671.Oct 6 2022, 12:51 AM
kadircet marked 2 inline comments as done.
  • Rename STL to Stdlib
This revision was landed with ongoing or failed builds.Oct 6 2022, 1:09 AM
This revision was automatically updated to reflect the committed changes.
thakis added a comment.Oct 6 2022, 6:43 AM

Thanks!

clang/lib/Tooling/Inclusions/STL/CMakeLists.txt
2 ↗(On Diff #465306)

FWIW, I'd move the header.

FWIW, I also don't think llvm's cmake setup is particularly unfortunate here either. It makes it easy to see where library boundaries are.

thakis added a comment.Oct 6 2022, 6:44 AM

(Alternatively, it looks like the only client is clangd, so maybe the file could just live in there instead of being in a dedicated library with a single client.)

kamaub added a subscriber: kamaub.Oct 7 2022, 10:46 AM

This change causes a linking failure during the check-all testing of 'clang-tools-extra' on the clang-ppc64le-rhel #22596 and clang-ppc64le-linux-multistage #23864 build bots, please address this failure as soon as possible.