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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.



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



================
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:
> 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?


================
Comment at: lib/Lex/HeaderSearch.cpp:753-754
+                  IncluderAndDir.second->getName()))
+            Diags.Report(IncludeLoc,
+                         diag::warn_quoted_include_in_framework_header)
+                << Filename;
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D47157