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