[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#1107210, @bruno wrote:

> > See also PR22165.
>
> Nice, seems related to this indeed. Are you aware of any development along those lines in clang-tidy? We would like this to be part of clang and be used as part of the normal compiler workflow, it can surely be as well part of any clang-tidy related include guidelines in the future.
>
> >> The warning is off by default.
> > 
> > We typically do not add off-by-default warnings because experience has shown the rarely get enabled in practice. Can you explain a bit more about why this one is off by default?
>
> Right. I believe this is going to be used in practice, the reason I'm adding it involves some user demand for such warning. Such quoted include use in frameworks happen often and we would like a smooth transition to happen here (e.g. do not initially affect -Werror users). If it proves worth it, we can migrate to on by default in the future. It wouldn't be a problem if we have it on by default on open source and disable by default downstream, but I rather be consistent.


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?



================
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;
----------------
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"`


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


================
Comment at: lib/Lex/HeaderSearch.cpp:873-874
+        !FoundByHeaderMap)
+      Diags.Report(IncludeLoc, diag::warn_quoted_include_in_framework_header)
+          << Filename;
+
----------------
Similar here.


Repository:
  rC Clang

https://reviews.llvm.org/D47157