dgoldman added inline comments.
================
Comment at: clang/lib/Lex/HeaderSearch.cpp:782
if (IsIncludeeInFramework) {
- NewInclude += ToFramework.str().drop_back(10); // drop .framework
- NewInclude += "/";
+ NewInclude += ToIncludeSpelling;
+ NewInclude += ">";
----------------
sammccall wrote:
> Nice!
>
> No good deed goes un-punished: it should be possible to add a diagnostic test?
> There are some under clang/test/Preprocessor/*framework*
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
================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1899
+ // symlink like `iPhoneSimulator14.5.sdk` while the file is instead
+ // located in `iPhoneSimulator.sdk` (the real folder).
+ if (NI->endswith(".sdk") && DI->endswith(".sdk")) {
----------------
sammccall wrote:
> 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)
Yep,
$ readlink
/Applications/Xcode_13.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.0.sdk
MacOSX.sdk
================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1962
+ } else {
+ // Prefer using `<UIKit/UIKit.h>` for non-`UIKit` main files.
+ //
----------------
sammccall wrote:
> 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.
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...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115183/new/
https://reviews.llvm.org/D115183
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
