This is an archive of the discontinued LLVM Phabricator instance.

Move the BySpelling map to IncludeStructure.
ClosedPublic

Authored by VitaNuo on Feb 7 2023, 8:53 AM.

Diff Detail

Event Timeline

VitaNuo created this revision.Feb 7 2023, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 8:53 AM
VitaNuo requested review of this revision.Feb 7 2023, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 8:53 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kadircet added inline comments.Feb 9 2023, 12:39 AM
clang-tools-extra/clangd/Headers.h
166

instead of building this on-demand, what about building it as we're collecting directives around https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L52 ?

afterwards we can just have a lookup function exposed here, that returns an ArrayRef<HeaderID> ?

VitaNuo updated this revision to Diff 496460.Feb 10 2023, 7:10 AM

Address review comments.

VitaNuo updated this revision to Diff 496463.Feb 10 2023, 7:18 AM

Expose API for lookup instead of retrieving the whole map.

Thanks for review!

clang-tools-extra/clangd/Headers.h
166

That's a great idea. I was thinking about this, but didn't know where to put it, so I thought if it was possible, you'd point out in review ;-)
Thanks!

kadircet added inline comments.Feb 10 2023, 8:32 AM
clang-tools-extra/clangd/Headers.cpp
76

right now we're only storing "resolved" includes from the main file and not all, this is creating a discrepancy between the view one gets through MainFileIncludes and through this map.
in addition to that only storing HeaderID gets the job done for IncludeCleaner, but won't really help anyone that wants to match main file includes apart from that (there's no easy way to go from a HeaderID to an Inclusion).

so instead of storing the HeaderID in the map values, we can actually store indexes into MainFileIncludes. later on during the lookup, we can build a SmallVector<Inclusion *> that contains pointers to the relevant includes. WDYT?

clang-tools-extra/clangd/Headers.h
166

what about renaming lookupHeaderIDsBySpelling to mainFileIncludesWithSpelling. with a comment explaining Returns includes inside the main file with the given Spelling. Spelling should include brackets or quotes, e.g. <foo>.

oh forgot to mention, could you also please add some tests into llvm-project/clang-tools-extra/clangd/unittests/HeadersTests.cpp ?

VitaNuo updated this revision to Diff 497602.Feb 15 2023, 2:34 AM
VitaNuo marked an inline comment as done.

Address review comments.

Thanks for the review!

clang-tools-extra/clangd/Headers.cpp
76

Ok sure.

kadircet added inline comments.Feb 16 2023, 8:41 AM
clang-tools-extra/clangd/Headers.h
167

we're still returning just the HeaderID, the suggestion was to return llvm::SmallVector<Inclusion*> so that applications can work with other information available in the Inclusion. we also won't have any requirements around include being resolved that way. the application should figure out what to do if HeaderID is missing.

also can you move this function body to source file instead?

VitaNuo updated this revision to Diff 498419.Feb 17 2023, 9:48 AM

Address comments

VitaNuo marked an inline comment as done.Feb 17 2023, 9:49 AM

Thanks for the comments!

VitaNuo updated this revision to Diff 498428.Feb 17 2023, 10:00 AM

Move to source file.

kadircet added inline comments.Feb 21 2023, 10:53 AM
clang-tools-extra/clangd/Headers.cpp
302

nit:

llvm::SmallVector<Inclusion*> Includes;
for (auto Idx : MainFileIncludesBySpelling.lookup(Spelling))
  Includes.push_back(&MainFileIncludes[Idx]);
return Includes;
clang-tools-extra/clangd/Headers.h
167

almost, but not right. we're returning copies of Inclusions here, not pointers to Inclusions inside the main file. the suggested signature was llvm::SmallVector<Inclusion*> any reason for returning by value?

clang-tools-extra/clangd/IncludeCleaner.cpp
516

nit:

if(!Inc.HeaderID.has_value())
  continue;
Used.insert(static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID));
VitaNuo updated this revision to Diff 499436.Feb 22 2023, 2:50 AM
VitaNuo marked 2 inline comments as done.

Address review comments.

Thanks for the comments!

clang-tools-extra/clangd/Headers.cpp
302

But lookup will return 0 if it doesn't find the spelling in the map (default value of int). And at the same time 0 is a valid index. Isn't that just wrong?

clang-tools-extra/clangd/Headers.h
167

Sure, thanks. No particular reason.

kadircet added inline comments.Feb 22 2023, 4:44 AM
clang-tools-extra/clangd/Headers.cpp
302

MainFileIncludesBySpelling is a map with values of type vector, not int. hence it'll be an empty vector, not 0.

VitaNuo updated this revision to Diff 499456.Feb 22 2023, 5:02 AM
VitaNuo marked an inline comment as done.

Address review comments.

VitaNuo marked 2 inline comments as done.Feb 22 2023, 5:02 AM
VitaNuo added inline comments.
clang-tools-extra/clangd/Headers.cpp
302

Oh right. Sorry I guess I got confused.

kadircet accepted this revision.Feb 22 2023, 5:07 AM

thanks (both for the patch and for bearing with me)!

This revision is now accepted and ready to land.Feb 22 2023, 5:07 AM
This revision was automatically updated to reflect the committed changes.
VitaNuo marked an inline comment as done.