[PATCH] D47301: Warning for framework include violation from Headers to PrivateHeaders

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno marked an inline comment as done.
bruno added inline comments.


================
Comment at: include/clang/Basic/DiagnosticGroups.td:34-35
 def AutoImport : DiagGroup<"auto-import">;
 def FrameworkHdrQuotedInclude : DiagGroup<"quoted-include-in-framework-header">;
+def FrameworkIncludePrivateFromPublic : DiagGroup<"framework-include-private-from-public">;
 def CXX14BinaryLiteral : DiagGroup<"c++14-binary-literal">;
----------------
vsapsai wrote:
> It might be convenient for users to have a warning group that will cover different framework warnings, something like -Wframework-hygiene. But it's out of scope for this review, more as an idea for future improvements.
Yep, that should come once we land all other bits.


================
Comment at: lib/Lex/HeaderSearch.cpp:625
+static bool isFrameworkStylePath(StringRef Path, bool &IsPrivateHeader,
+                                 SmallVectorImpl<char> &FrameworkName) {
   using namespace llvm::sys;
----------------
vsapsai wrote:
> In this function we accept some paths that aren't valid framework paths. Need to think more if it is OK or if we want to be stricter.
It should be ok at this point, otherwise the framework style path would have failed before finding this header.


================
Comment at: test/Modules/framework-public-includes-private.m:33
+
+int bar() { return foo(); }
+
----------------
vsapsai wrote:
> I'm not entirely sure it's not covered by existing test. It might be worth testing private header including public header and private header including another private header.
The warning is on by default and clang already have the scenario you described in other module tests - those would fail if there's a bug in the logic here.


https://reviews.llvm.org/D47301