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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D47157#1115445, @bruno wrote:

> > Consistency would be nice, but at the same time, I don't see a good metric for when we'd know it's time to switch it to being on by default. I'm worried that it'll remain off by default forever simply because no one thinks to go turn it on (because it's silent by default). Perhaps on-by-default here and off-by-default downstream would be the better approach, or do you think this would be too disruptive to enable by default anywhere?
>
> I believe this could be too disruptive to enable now, since it's still very common to find quoted includes in framework headers. This is very important for Modules to properly work with frameworks (quoted headers are usually considered non-modular when modules builds kick in) and is actually very compelling for us to turn it on by default on Darwin as soon as we can, but we need to educate users first.


That sounds like good justification for it being off by default then, thank you.



================
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;
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D47157