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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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?



================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1962
+    } else {
+      // Prefer using `<UIKit/UIKit.h>` for non-`UIKit` main files.
+      //
----------------
dgoldman wrote:
> 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...
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.


================
Comment at: clang/test/Modules/double-quotes.m:27
 // CHECK: double-quoted include "A0.h" in framework header, expected angle-bracketed instead
+// CHECK: <A/A0.h>
 // CHECK: double-quoted include "B.h" in framework header, expected angle-bracketed instead
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115183/new/

https://reviews.llvm.org/D115183