[PATCH] D115183: [clang][HeaderSearch] Support framework includes in suggestPath...

[David Goldman via Phabricator via cfe-commits]

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