This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Rollforward include-cleaner library usage in symbol collector.
ClosedPublic

Authored by VitaNuo on Jul 31 2023, 2:52 AM.

Diff Detail

Event Timeline

VitaNuo created this revision.Jul 31 2023, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 2:52 AM
VitaNuo requested review of this revision.Jul 31 2023, 2:52 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 31 2023, 2:52 AM
kadircet added inline comments.Sep 4 2023, 11:17 PM
clang-tools-extra/clangd/index/SymbolCollector.cpp
848–860

can you add a comment here saying, We update providers for a symbol with each occurence, as SymbolCollector might run while parsing, rather than at the end of a translation unit. Hence we see more and more redecls over time.

917

we should keep the getStdHeaders logic for objc

939

can you add a // FIXME: Get rid of this once include-cleaner has support for system headers. to this branch

942

again a comment saying For physical files, prefer URIs as spellings might change depending on the translation unit.

clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
129

can you also add comment move(ExecutionPolicy&& policy, ForwardIt1 first, ForwardIt1 last, ForwardIt2 d_first ); and move this change into a separate patch with a test case in clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp?

clang/lib/Tooling/Inclusions/Stdlib/StdSpecialSymbolMap.inc
365 ↗(On Diff #545562)

can you move this into a separate patch?

VitaNuo updated this revision to Diff 556004.Sep 6 2023, 4:32 AM
VitaNuo marked 5 inline comments as done.

Address review comments.

VitaNuo added inline comments.Sep 7 2023, 4:40 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
917

getStdHeaders returns empty string for Obj-C, are you sure you meant it?

kadircet added inline comments.Sep 7 2023, 4:52 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
917

it reads a little bit weird, but it still actually works, for objective-c++ to be more specific. as we'll have LangOpts.CPlusPlus set for objc++

VitaNuo updated this revision to Diff 556130.Sep 7 2023, 5:08 AM
VitaNuo marked 2 inline comments as done.

Address the comment.

kadircet added inline comments.Sep 7 2023, 5:59 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
828

sorry i got confused, this also works for ObjC, not just ObjC++. we set C-like features for ObjectiveC. can you just restore as-is?

919

i guess LHS of the assignment got dropped by mistake?

VitaNuo marked 2 inline comments as done.Sep 7 2023, 6:03 AM
VitaNuo added inline comments.
clang-tools-extra/clangd/index/SymbolCollector.cpp
919

Sorry, juggling unconnected stuff makes me inattentive.

VitaNuo updated this revision to Diff 556135.Sep 7 2023, 6:04 AM
VitaNuo marked an inline comment as done.

Address comments.

kadircet accepted this revision.Sep 10 2023, 10:20 PM

thanks!

This revision is now accepted and ready to land.Sep 10 2023, 10:20 PM