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 <angled> include" + >, InGroup<FrameworkHdrQuotedInclude>, DefaultIgnore; ---------------- bruno wrote: > dexonsmith wrote: > > bruno wrote: > > > dexonsmith wrote: > > > > 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). > > > I haven't measured, but for each quoted include we would have to: > > > > > > - Start a fresh header search. > > > - Look for `Foo.h` in all possible frameworks in the path (just on the > > > Darwin macOS SDK path that would be around 140 frameworks). > > > - If it's only found in once place, we are mostly safe to say we found a > > > matching framework, otherwise we can't emit a reliable fixit. > > > - Header maps and VFS might add extra level of searches (this is very > > > common in Xcode based clang invocations). > > Can we just check if it's a header in the *same* framework? > For some pretty obvious cases we can probably assume that this is what the > user wants, but even so it might be misleading. For example, if you're in > `ABC/H1.h`, you include `H2.h` and the framework ABC has an `ABC/H2.h`. It > could be that `#include "H2.h"` is mapped via header maps to `$SOURCE/H2.h` > instead of using the installed headers in the framework style build products. > > This is likely a mistake, but what if it's intentional? I would prefer if the > user rethink it instead of just buying potential misleading clues. OTOH, I > share the concern that we don't need to be perfect here and only emit the > fixit for really obvious cases, and not for the others. Will update the patch > to include a fixit to the very straightforward scenario: `H2.h` was found in > the same framework style path as the includer. > Will update the patch to include a fixit to the very straightforward > scenario: H2.h was found in the same framework style path as the includer. Thanks, I think users will appreciate the compiler helping them out like that. ================ Comment at: lib/Lex/HeaderSearch.cpp:753-754 + IncluderAndDir.second->getName())) + Diags.Report(IncludeLoc, + diag::warn_quoted_include_in_framework_header) + << Filename; ---------------- dexonsmith wrote: > 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. That makes sense to me. 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
