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; ---------------- 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? 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
