[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:
> bruno wrote:
> > 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).
> Can we just check if it's a header in the *same* framework?
For some pretty obvious cases we can probably assume that this is what the user wants, but even so it might be misleading. For example, if you're in `ABC/H1.h`, you include `H2.h` and the framework ABC has an `ABC/H2.h`. It could be that `#include "H2.h"` is mapped via header maps to `$SOURCE/H2.h` instead of using the installed headers in the framework style build products.

This is likely a mistake, but what if it's intentional? I would prefer if the user rethink it instead of just buying potential misleading clues. OTOH, I share the concern that we don't need to be perfect here and only emit the fixit for really obvious cases, and not for the others. Will update the patch to include a fixit to the very straightforward scenario: `H2.h` was found in the same framework style path as the includer.


Repository:
  rC Clang

https://reviews.llvm.org/D47157