This is an archive of the discontinued LLVM Phabricator instance.

[HeaderSearch] Track framework name in LookupFile
ClosedPublic

Authored by dgoldman on Jan 20 2022, 12:33 PM.

Details

Summary

Previously, the Framework name was only set if the file
came from a header mapped framework; now we'll always
set the framework name if the file is in a framework.

Diff Detail

Event Timeline

dgoldman requested review of this revision.Jan 20 2022, 12:33 PM
dgoldman created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 12:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sammccall resigned from this revision.Jan 21 2022, 11:20 AM
sammccall added subscribers: jansvoboda11, cishida.

Thanks for splitting this out from the other patch. This feels subtle and I don't think I know enough about frameworks to review.

@cishida made a recent change here which @jansvoboda11 reviewed - I'm guessing they'd know what to do here.

clang/lib/Lex/HeaderSearch.cpp
1043

The comment here explicitly say "in a header map and". Unfortunately it doesn't say why, and I don't know enough to guess.

Cyndy and Jan, hopefully ya'll are the right reviewers here? We're using this Framework information to know to compute framework style includes in https://reviews.llvm.org/D117056 for clangd, please LMK if this is a bad idea and we should do something else.

ormris removed a subscriber: ormris.Jan 24 2022, 11:08 AM

My understanding for the header map restriction is because headermaps are generally emitted once for a framework build and only consumed to build such framework and this struct information is only queried during this.
It sounds like for the clangd support, you're not as interested in what framework is being built, but the name of the framework when included via framework style in any context. Is that correct? It seems fine to extend its use cases. @jansvoboda11 What do you think?

My understanding for the header map restriction is because headermaps are generally emitted once for a framework build and only consumed to build such framework and this struct information is only queried during this.
It sounds like for the clangd support, you're not as interested in what framework is being built, but the name of the framework when included via framework style in any context. Is that correct? It seems fine to extend its use cases. @jansvoboda11 What do you think?

Yep, exactly, for clangd, we want to know when a header is included via a framework style include (although maybe it would be better to know if a header is actually apart of a framework regardless of the spelling to import it?) so we can generate a proper framework style include for symbols in the framework. That lets us suggest an include of <UIKit/UIKit.h> for UIView, for example.

cishida accepted this revision.Jan 24 2022, 4:32 PM

I followed up about this approach with folks more familiar with SourceKit and seems fine, so LGTM. I'd wait a day to see if Jan has any concerns.

clang/lib/Lex/HeaderSearch.cpp
1043

nit/reminder: This should be updated now

This revision is now accepted and ready to land.Jan 24 2022, 4:32 PM

I don't have any suggestions here except for running the patch through clang-format.

dgoldman updated this revision to Diff 403340.Jan 26 2022, 11:01 AM

Update comment + run clang-format

dgoldman marked 2 inline comments as done.Jan 26 2022, 11:02 AM
dgoldman added inline comments.
clang/lib/Lex/HeaderSearch.cpp
1043

Updated, LMK if you have any suggestions on the wording here.

cishida added inline comments.Jan 26 2022, 11:12 AM
clang/lib/Lex/HeaderSearch.cpp
1043

LGTM

This revision was automatically updated to reflect the committed changes.
dgoldman marked an inline comment as done.