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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai 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">;
----------------
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.


================
Comment at: lib/Lex/HeaderSearch.cpp:625
+static bool isFrameworkStylePath(StringRef Path, bool &IsPrivateHeader,
+                                 SmallVectorImpl<char> &FrameworkName) {
   using namespace llvm::sys;
----------------
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.


================
Comment at: lib/Lex/HeaderSearch.cpp:893
+      // from Foo.framework/PrivateHeaders, since this violates public/private
+      // api boundaries and can cause modular dependency cycles.
+      SmallString<128> ToFramework;
----------------
s/api/API/


================
Comment at: lib/Lex/HeaderSearch.cpp:895
+      SmallString<128> ToFramework;
+      bool IncludeIsFrameworkPrivateHeader = false;
+      if (IsIncluderFromFramework && !IsFrameworkPrivateHeader &&
----------------
My opinion on readability is personal, so take it with a grain of salt. What if you make variable names more consistent, like
* IsIncluderPrivateHeader
* IsIncluderFromFramework
* IsIncludeePrivateHeader


================
Comment at: test/Modules/framework-public-includes-private.m:33
+
+int bar() { return foo(); }
+
----------------
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.


https://reviews.llvm.org/D47301