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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
