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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith 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;
----------------
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).


================
Comment at: lib/Lex/HeaderSearch.cpp:753-754
+                  IncluderAndDir.second->getName()))
+            Diags.Report(IncludeLoc,
+                         diag::warn_quoted_include_in_framework_header)
+                << Filename;
----------------
bruno wrote:
> dexonsmith wrote:
> > bruno wrote:
> > > aaron.ballman wrote:
> > > > This seems like a good place for a fix-it to switch the include style. Is there a reason to not do that work for the user?
> > > Like I explained above, we don't know which framework the header could be part of, so a fix-it could be misleading.
> > Clang supports editor placeholders, which we use in some refactoring-style fix-its.  I think this would be spelled `<#framework-name#>`, or `#include <<#framework-name#>/Foo.h>`
> My current understanding (after chatting with @arphaman) is that editor placeholders isn't a great fit here:
> 
> - For non IDE uses of this, it will just be weird to output something like `#include <<#framework-name#>/Foo.h>`. Even if we wanted to emit this only for IDE use, clang currently has no way to make that distinction (editor placeholder related compiler flags only make sense when actually making the special token sequence lexable, not when generating it)
> - Fixits are (with some known exceptions) meant to be applied to code and subsequently allow compilation to succeed, this wouldn't be the case here.
Okay, sounds good.


Repository:
  rC Clang

https://reviews.llvm.org/D47157