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 <angled> include" + >, InGroup<FrameworkHdrQuotedInclude>, DefaultIgnore; ---------------- aaron.ballman wrote: > 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. I'm also interested in just how expensive it would be, because I think users will be frustrated that the compiler knows it's a framework include "so it obviously knows which framework". I'd be fine if the fix-it came in a follow-up commit though (not sure how Aaron feels). ================ Comment at: lib/Lex/HeaderSearch.cpp:753-754 + IncluderAndDir.second->getName())) + Diags.Report(IncludeLoc, + diag::warn_quoted_include_in_framework_header) + << Filename; ---------------- bruno wrote: > dexonsmith wrote: > > bruno wrote: > > > 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. > > Clang supports editor placeholders, which we use in some refactoring-style > > fix-its. I think this would be spelled `<#framework-name#>`, or `#include > > <<#framework-name#>/Foo.h>` > My current understanding (after chatting with @arphaman) is that editor > placeholders isn't a great fit here: > > - For non IDE uses of this, it will just be weird to output something like > `#include <<#framework-name#>/Foo.h>`. Even if we wanted to emit this only > for IDE use, clang currently has no way to make that distinction (editor > placeholder related compiler flags only make sense when actually making the > special token sequence lexable, not when generating it) > - Fixits are (with some known exceptions) meant to be applied to code and > subsequently allow compilation to succeed, this wouldn't be the case here. Okay, sounds good. 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
