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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
