This is an archive of the discontinued LLVM Phabricator instance.

[clang][HeaderSearch] Support framework includes in suggestPath...
ClosedPublic

Authored by dgoldman on Dec 6 2021, 1:36 PM.

Details

Summary

Clang will now search through the framework includes to identify
the framework include path to a file, and then suggest a framework
style include spelling for the file.

Diff Detail

Event Timeline

dgoldman requested review of this revision.Dec 6 2021, 1:36 PM
dgoldman created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2021, 1:36 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dgoldman updated this revision to Diff 392176.Dec 6 2021, 2:05 PM

Fix IncludeSpelling issue

dgoldman updated this revision to Diff 392180.Dec 6 2021, 2:13 PM

Use consistent IsSystem detection

dgoldman updated this revision to Diff 392477.Dec 7 2021, 11:06 AM

Add HeaderSearch tests

dexonsmith resigned from this revision.Dec 8 2021, 3:30 PM
dexonsmith added reviewers: vsapsai, jansvoboda11.

This looks pretty nice to me, but I'm not convinced it's a good idea to include the umbrella header heuristic.

clang/lib/Lex/HeaderSearch.cpp
782–786

Nice!

No good deed goes un-punished: it should be possible to add a diagnostic test?
There are some under clang/test/Preprocessor/*framework*

1862

Seems like it'd be more consistent with the existing IsSystem to expand the callsites as

if (... && CheckDir(...)) {
  if (IsSystem)
    *IsSystem = whatever;
  IsFramework = whatever;
}

instead of taking a param here. (And honestly the CheckDir() && IsSystem thing is confusing, so it'd help)

1899

Nice comment, very helpful!

Just wanted to check you're sure the versioned one is the symlink and the unversioned one is the real folder, rather than the other way. (Plausible, just surprising to me)

1916

nit: /*IsFramework=*/, one star no spaces

1962

This heuristic looks iffy to me, it's not just trying to help the caller insert the right header, but tell them they want a different one for style reasons.

The first example framework in https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/FrameworkAnatomy.html doesn't include an umbrella header of this form.

Moreover, non-framework libraries have umbrella headers too, this seems like an orthogonal concern best caught in some other way.

dgoldman updated this revision to Diff 397733.Jan 5 2022, 4:12 PM
dgoldman marked 4 inline comments as done.

Fixes for review - update test

dgoldman added inline comments.Jan 5 2022, 4:15 PM
clang/lib/Lex/HeaderSearch.cpp
782–786

I couldn't seem to get it to generate a warning for a subframework, for both there and in double-quotes.m under Modules, so instead I added a simple test to headersearch for the sub framework case and made sure the fix it in double-quotes.m was OK for the existing cases

1899

Yep,

$ readlink /Applications/Xcode_13.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.0.sdk
MacOSX.sdk

1962

See https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Tasks/IncludingFrameworks.html, it's common to use this umbrella header (especially for Apple SDKs) and some frameworks require it (as opposed to including the header directly), thus I think this default makes sense even if it isn't 100%.

Any other ideas on how we can help here? I guess if we had FS access we could see if the umbrella header exists...

Thanks, test seems sensible.
I remain unconvinced that the umbrella stuff belongs here.
Maybe pull it out of this patch and send it separately if you want to discuss further, or get more opinions?

clang/lib/Lex/HeaderSearch.cpp
1962

I get that it's often a good idea/mandatory to include the umbrella header (for many libraries, not just frameworks), but it's not this function's job to choose which file to include, it's to find the best spelling of the file that was requested.

Similarly it doesn't check if the filename is "bits/shared_ptr.h" and suggest <shared_ptr> instead.

Any other ideas on how we can help here?

It depends what you're trying to achieve, maybe write a clang-tidy check instead? Or if this is something the compiler itself needs to care about, write a separate function to do the heuristic correction.
If you need the spelled, heuristically-corrected name, you can correct it first and then call this code, rather than having the spelling code do heuristic correction.

clang/test/Modules/double-quotes.m
27 ↗(On Diff #397733)

it's hard to understand the context of how these headers are named, is there any other text on the line you can include in the CHECK?

dgoldman updated this revision to Diff 397882.Jan 6 2022, 7:22 AM

Improve double-quotes.m test

dgoldman marked an inline comment as done.Jan 6 2022, 7:22 AM
dgoldman updated this revision to Diff 398178.Jan 7 2022, 9:30 AM

Don't suggest umbrella headers

dgoldman marked an inline comment as done.Jan 7 2022, 9:33 AM
sammccall accepted this revision.Jan 7 2022, 3:19 PM
sammccall added inline comments.
clang/lib/Lex/HeaderSearch.cpp
1951

nit: I don't think these strings need to be so big, framework includes are a fairly uncommon case probably?

This revision is now accepted and ready to land.Jan 7 2022, 3:19 PM
sammccall added inline comments.Jan 7 2022, 3:20 PM
clang/lib/Lex/HeaderSearch.cpp
1951

nvm, not sure where I got that idea.

This revision was landed with ongoing or failed builds.Jan 10 2022, 9:28 AM
This revision was automatically updated to reflect the committed changes.