This revision was automatically updated to reflect the committed changes.
Closed by commit rL335184: Warning for framework headers using double quote
includes (authored by bruno, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
bruno added inline comments.
Comment at: test/Modules/double-quotes.m:24-25
+
+// The same warnings show up when modules is on but -verify doesn't get it
+// because they only show up under the module A building context.
+// RUN: FileCheck --input-file=%t/stderr %s
bruno updated this revision to Diff 149359.
bruno edited the summary of this revision.
bruno added a comment.
Update after Duncan's review: remove header name from the warning message
(since it's already in the fixit)
https://reviews.llvm.org/D47157
Files:
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D47157
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dexonsmith added inline comments.
Comment at: test/Modules/double-quotes.m:27-29
+// CHECK: double-quoted include "A0.h" in framework header, expected
angle-bracketed include instead
+// CHECK: double-quoted include "B.h" in framework header, expected
angle-bracketed include
aaron.ballman added inline comments.
Comment at: test/Modules/double-quotes.m:27-29
+// CHECK: double-quoted include "A0.h" in framework header, expected
angle-bracketed include instead
+// CHECK: double-quoted include "B.h" in framework header, expected
angle-bracketed
dexonsmith added inline comments.
Comment at: test/Modules/double-quotes.m:24-25
+
+// The same warnings show up when modules is on but -verify doesn't get it
+// because they only show up under the module A building context.
+// RUN: FileCheck --input-file=%t/stderr %s
bruno updated this revision to Diff 149313.
bruno added a comment.
Updated the patch after Duncan and Aaron reviews. I actually went a bit more
aggressive with the fixits, since I realized the conditions for the warning are
already strict enough and we should take the chance to be more clear.
aaron.ballman 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
include"
+ >, InGroup, DefaultIgnore;
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
include"
+ >, InGroup, DefaultIgnore;
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
include"
+ >, InGroup, DefaultIgnore;
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
include"
+ >, InGroup, DefaultIgnore;
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
include"
+ >, InGroup, DefaultIgnore;
bruno added a subscriber: arphaman.
bruno added inline comments.
Comment at: lib/Lex/HeaderSearch.cpp:753-754
+ IncluderAndDir.second->getName()))
+Diags.Report(IncludeLoc,
+ diag::warn_quoted_include_in_framework_header)
+
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
dexonsmith added inline comments.
Comment at: lib/Lex/HeaderSearch.cpp:753-754
+ IncluderAndDir.second->getName()))
+Diags.Report(IncludeLoc,
+ diag::warn_quoted_include_in_framework_header)
+<< Filename;
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
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
Eugene.Zelenko added a comment.
In https://reviews.llvm.org/D47157#1110430, @bruno wrote:
> Hi Eugene,
>
> > You could just not include new warning into -Wall and -Wextra. Those who
> > will want to check #include statements correctness could enable it
> > explicitly.
>
> This is exactly
bruno added a comment.
Hi Eugene,
> You could just not include new warning into -Wall and -Wextra. Those who will
> want to check #include statements correctness could enable it explicitly.
This is exactly what's happening in the patch, the new warning isn't part of
`-Wall` or `-Wextra`, and
Eugene.Zelenko added a comment.
>>> 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
bruno added a comment.
> 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
aaron.ballman added a comment.
> 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?
Repository:
rC Clang
Eugene.Zelenko added a comment.
See also PR22165.
Repository:
rC Clang
https://reviews.llvm.org/D47157
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bruno added a comment.
Depends on https://reviews.llvm.org/D46485
Repository:
rC Clang
https://reviews.llvm.org/D47157
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
bruno created this revision.
bruno added reviewers: dexonsmith, ributzka, steven_wu.
Introduce -Wquoted-include-in-framework-header, which should fire
a warning whenever a quote include appears in a framework header,
example, for A.h added in the tests:
...
A.framework/Headers/A.h:2:10: warning:
26 matches
Mail list logo
