This will allow moving the IncludeCleaner library essentials to Clang
and decoupling them from the majority of clangd.
The patch itself just moves the code, it doesn't change existing
functionality.
Paths
| Differential D119130
[clangd] NFC: Move stdlib headers handling to Clang ClosedPublic Authored by kbobyrev on Feb 7 2022, 4:21 AM.
Details Summary This will allow moving the IncludeCleaner library essentials to Clang The patch itself just moves the code, it doesn't change existing
Diff Detail
Event TimelineHerald added subscribers: usaxena95, kadircet, arphaman, mgorny. · View Herald TranscriptFeb 7 2022, 4:21 AM Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 7 2022, 4:21 AM kbobyrev retitled this revision from [clangd] Move stdlibg headers handling to Clang to [clangd] NFC: Move stdlibg headers handling to Clang.Feb 7 2022, 4:21 AM Comment Actions The moved *.inc files are generated, the generator needs to be moved (and updated?) too I think. Is having clangd continue to include the .inc files textually intended to stay that way, or is it a FIXME? Comment Actions This looks sensible to me, but as I wrote this code we should maybe get a second look (@kadircet?) that it makes sense to lift to clang::tooling. Some positive experience: we've used it successfully in an internal clang-tidy check.
Comment Actions Regarding the include mapping generator, I think it would've been better if we had some sort of list directly from libc++ (as this is now being part of clang rather than just clangd), but having the current symbol mapping available for other tools too is definitely a useful improvement and implementation details can change later on. As for stdlib symbol/header/recognizer, I've got a couple questions around the functionality we are exposing:
kbobyrev marked 9 inline comments as done. Comment ActionsAddress review comments.
Comment Actions
Right, I agree that using libc++ directly would be better, but for now this just makes the functionality public. We can change it and update as we want afterwards.
kbobyrev retitled this revision from [clangd] NFC: Move stdlibg headers handling to Clang to [clangd] NFC: Move stdlib headers handling to Clang.Feb 8 2022, 6:29 AM Comment Actions Oh, and also regarding the Python generator scripts: they don't seem to work with the latest archives :( 2018 mentioned in the docs (works perfectly) but 2019 crashes. Probably not very relevant if we switch to libc++ eventually. Comment Actions
Yikes, we should fix that. Especially since the 2019 version is 3 years out of date already...
This seems pretty unlikely to me. This revision is now accepted and ready to land.Feb 9 2022, 1:43 AM Comment Actions
Agreed! Though, the other problem is that it seems that nothing beyond 2019 version really exists. Which means that even if we support that one, it's unlikely to be up-to-date. For us to be relatively up-to-date we might need to ask someone working on cppreference for new archives and the format is likely to be different from 2019, too, so I don't know if we should fix the issue now vs. try to get newer archives and fix the issue after that.
This revision was landed with ongoing or failed builds.Feb 9 2022, 2:05 AM Closed by commit rG46a6f5ae148a: [clangd] NFC: Move stdlib headers handling to Clang (authored by kbobyrev). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions After this commit I am seeing the link time error ld.lld: error: undefined symbol: clang::DeclContext::isInlineNamespace() const >>> referenced by StandardLibrary.cpp:118 (/home/pmw/projects/upstream-llvm/llvm-project/clang/lib/Tooling/Inclusions/StandardLibrary.cpp:118) >>> tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::tooling::stdlib::Recognizer::namespaceSymbols(clang::NamespaceDecl const*)::'lambda'()::operator()() const) ld.lld: error: undefined symbol: clang::Decl::castFromDeclContext(clang::DeclContext const*) >>> referenced by DeclBase.h:2562 (/home/pmw/projects/upstream-llvm/llvm-project/clang/include/clang/AST/DeclBase.h:2562) >>> tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::cast_convert_decl_context<clang::Decl, false>::doit(clang::DeclContext*)) >>> referenced by DeclBase.h:2558 (/home/pmw/projects/upstream-llvm/llvm-project/clang/include/clang/AST/DeclBase.h:2558) >>> tools/clang/lib/Tooling/Inclusions/CMakeFiles/obj.clangToolingInclusions.dir/StandardLibrary.cpp.o:(clang::cast_convert_decl_context<clang::NamedDecl, false>::doit(clang::DeclContext const*)) If I revert this commit it goes away. I've tried a fresh cmake and build and get the same result. Comment Actions
Thanks for reporting! This should be fixed in https://github.com/llvm/llvm-project/commit/e3ba831937189ec61e28ced0b0bfc1e41a3510f8, your issue looks the same I found in a failing buildbot; could you please try again and let me know if it's still broken? Comment Actions This makes clang-format depend on clang's AST library (because Format depends on Tooling/Inclusions, and this makes Tooling/Inclusions depend on AST), which it shouldn't. Could this move into a dedicated library, or in some library that Format doesn't depend on? Comment Actions Or, alternatively, maybe Recognizer could stay in clangd (or move to a separate lib); looks like that's the only thing in StandardLibrary.h that needs clang/AST/Decl.h. Comment Actions
Oops, definitely an unintended outcome. Comment Actions yeah sorry for the mess here. i agree that we should move StandardLibrary to its own library. i'll try to take a stab at this tomorrow, if no one does it before then.
Revision Contents
Diff 407096 clang-tools-extra/clangd/CSymbolMap.inc
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/IncludeCleaner.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/StdSymbolMap.inc
clang-tools-extra/clangd/include-mapping/cppreference_parser.py
clang-tools-extra/clangd/include-mapping/gen_std.py
clang-tools-extra/clangd/include-mapping/test.py
clang-tools-extra/clangd/index/CanonicalIncludes.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
clang/include/clang/Tooling/Inclusions/CSymbolMap.inc
clang/include/clang/Tooling/Inclusions/StandardLibrary.h
clang/include/clang/Tooling/Inclusions/StdSymbolMap.inc
clang/lib/Tooling/Inclusions/CMakeLists.txt
clang/lib/Tooling/Inclusions/StandardLibrary.cpp
clang/tools/include-mapping/cppreference_parser.py
clang/tools/include-mapping/gen_std.py
clang/tools/include-mapping/test.py
clang/unittests/Tooling/CMakeLists.txt
clang/unittests/Tooling/StandardLibraryTest.cpp
|
Recognizer test also needs to be moved