[PATCH] D47157: Warning for framework headers using double quote includes

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added inline comments.


================
Comment at: include/clang/Basic/DiagnosticLexKinds.td:713
+def warn_quoted_include_in_framework_header : Warning<
+  "double-quoted include \"%0\" in framework header, expected system style <angled> include"
+  >, InGroup<FrameworkHdrQuotedInclude>, DefaultIgnore;
----------------
dexonsmith wrote:
> aaron.ballman wrote:
> > bruno wrote:
> > > aaron.ballman wrote:
> > > > 80-col limit?
> > > > 
> > > > Also, I'd probably drop "system style" and reword slightly to:
> > > > 
> > > > `"double-quoted include \"%0\" in framework header, expected angle-bracketed include <%0> instead"`
> > > Unfortunately this won't work because for framework style includes we use the angled-bracketed with the framework name. For example, if one wants to include `Foo.h` from `Foo.framework`, one should use `#include <Foo/Foo.h>`, although on disk you actually have `Foo.framework/Headers/Foo.h`. Framework header lookup does this magic and other similar ones.
> > > 
> > > Since we don't know which framework the quoted header could be part of, it was not included in the warning (doing so would require extra header searches - which could be expensive for this specific warning). However it seems that I can do better to indicate that the framework name is desired here, perhaps:
> > > 
> > > `"double-quoted include \"%0\" in framework header, expected angle-bracketed include <FrameworkName/%0> instead"`
> > > 
> > > How does that sound to you? Other suggestions?
> > Thank you for the explanation!
> > 
> > I think your suggested text sounds good, though I do wonder how expensive is "expensive" in finding the intended header? Not only would it provide a better diagnostic, it would also let you use a fixit that doesn't use editor placeholders.
> I'm also interested in just how expensive it would be, because I think users will be frustrated that the compiler knows it's a framework include "so it obviously knows which framework".
> 
> I'd be fine if the fix-it came in a follow-up commit though (not sure how Aaron feels).
I haven't measured, but for each quoted include we would have to:

- Start a fresh header search.
- Look for `Foo.h` in all possible frameworks in the path (just on the Darwin macOS SDK path that would be around 140 frameworks).
- If it's only found in once place, we are mostly safe to say we found a matching framework, otherwise we can't emit a reliable fixit.
- Header maps and VFS might add extra level of searches (this is very common in Xcode based clang invocations).


Repository:
  rC Clang

https://reviews.llvm.org/D47157