bruno updated this revision to Diff 152198.
bruno added a comment.
Address Volodymyr's comments
https://reviews.llvm.org/D47301
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticLexKinds.td
lib/Lex/HeaderSearch.cpp
test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h
test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap
test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap
test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h
test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h
test/Modules/Inputs/framework-public-includes-private/a.hmap.json
test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h
test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap
test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap
test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h
test/Modules/Inputs/framework-public-includes-private/z.hmap.json
test/Modules/Inputs/framework-public-includes-private/z.yaml
test/Modules/framework-public-includes-private.m
Index: test/Modules/framework-public-includes-private.m
===================================================================
--- /dev/null
+++ test/Modules/framework-public-includes-private.m
@@ -0,0 +1,37 @@
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// RUN: hmaptool write %S/Inputs/framework-public-includes-private/a.hmap.json %t/a.hmap
+// RUN: hmaptool write %S/Inputs/framework-public-includes-private/z.hmap.json %t/z.hmap
+
+// RUN: sed -e "s:TEST_DIR:%S/Inputs/framework-public-includes-private:g" \
+// RUN: %S/Inputs/framework-public-includes-private/z.yaml > %t/z.yaml
+
+// The output with and without modules should be the same, without modules first.
+// RUN: %clang_cc1 \
+// RUN: -iquote %t/z.hmap -iquote %t/a.hmap -ivfsoverlay %t/z.yaml \
+// RUN: -F%S/Inputs/framework-public-includes-private \
+// RUN: -fsyntax-only %s -verify
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN: -iquote %t/z.hmap -iquote %t/a.hmap -ivfsoverlay %t/z.yaml \
+// RUN: -F%S/Inputs/framework-public-includes-private \
+// RUN: -fsyntax-only %s \
+// RUN: 2>%t/stderr
+
+// The same warnings show up when modules is on but -verify doesn't get it
+// because they only show up under the module A building context.
+// RUN: FileCheck --input-file=%t/stderr %s
+// CHECK: public framework header includes private framework header 'A/APriv.h'
+// CHECK: public framework header includes private framework header 'A/APriv2.h'
+// CHECK: public framework header includes private framework header 'Z/ZPriv.h'
+
+#import "A.h"
+
+int bar() { return foo(); }
+
+// expected-warning@Inputs/framework-public-includes-private/A.framework/Headers/A.h:2{{public framework header includes private framework header 'A/APriv.h'}}
+// expected-warning@Inputs/framework-public-includes-private/A.framework/Headers/A.h:3{{public framework header includes private framework header 'A/APriv2.h'}}
+// expected-warning@Inputs/framework-public-includes-private/flat-header-path/Z.h:1{{public framework header includes private framework header 'Z/ZPriv.h'}}
Index: test/Modules/Inputs/framework-public-includes-private/z.yaml
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/z.yaml
@@ -0,0 +1,45 @@
+{
+ 'version': 0,
+ 'case-sensitive': 'false',
+ 'use-external-names' : 'false',
+ 'roots': [
+ {
+ 'type': 'directory',
+ 'name': "TEST_DIR/Z.framework/Headers",
+ 'contents': [
+ {
+ 'type': 'file',
+ 'name': "Z.h",
+ 'external-contents': "TEST_DIR/flat-header-path/Z.h"
+ }
+ ]
+ },
+ {
+ 'type': 'directory',
+ 'name': "TEST_DIR/Z.framework/PrivateHeaders",
+ 'contents': [
+ {
+ 'type': 'file',
+ 'name': "ZPriv.h",
+ 'external-contents': "TEST_DIR/flat-header-path/ZPriv.h"
+ }
+ ]
+ },
+ {
+ 'type': 'directory',
+ 'name': "TEST_DIR/Z.framework/Modules",
+ 'contents': [
+ {
+ 'type': 'file',
+ 'name': "module.modulemap",
+ 'external-contents': "TEST_DIR/flat-header-path/Z.modulemap"
+ },
+ {
+ 'type': 'file',
+ 'name': "module.private.modulemap",
+ 'external-contents': "TEST_DIR/flat-header-path/Z.private.modulemap"
+ }
+ ]
+ }
+ ]
+}
Index: test/Modules/Inputs/framework-public-includes-private/z.hmap.json
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/z.hmap.json
@@ -0,0 +1,7 @@
+
+{
+ "mappings" :
+ {
+ "ZPriv.h" : "Z/ZPriv.h"
+ }
+}
Index: test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/flat-header-path/ZPriv.h
@@ -0,0 +1 @@
+// ZPriv.h
Index: test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.private.modulemap
@@ -0,0 +1,3 @@
+framework module Z_Private {
+ header "ZPriv.h"
+}
Index: test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.modulemap
@@ -0,0 +1,3 @@
+framework module Z {
+ header "Z.h"
+}
Index: test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/flat-header-path/Z.h
@@ -0,0 +1 @@
+#import "ZPriv.h"
Index: test/Modules/Inputs/framework-public-includes-private/a.hmap.json
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/a.hmap.json
@@ -0,0 +1,8 @@
+
+{
+ "mappings" :
+ {
+ "A.h" : "A/A.h",
+ "APriv2.h" : "A/APriv2.h"
+ }
+}
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv2.h
@@ -0,0 +1 @@
+// APriv.h
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/PrivateHeaders/APriv.h
@@ -0,0 +1 @@
+// APriv.h
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.private.modulemap
@@ -0,0 +1,3 @@
+framework module A_Private {
+ header "APriv.h"
+}
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/Modules/module.modulemap
@@ -0,0 +1,4 @@
+
+framework module A {
+ header "A.h"
+}
Index: test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/framework-public-includes-private/A.framework/Headers/A.h
@@ -0,0 +1,5 @@
+
+#include <A/APriv.h>
+#include "APriv2.h"
+#include <Z/Z.h>
+int foo();
Index: lib/Lex/HeaderSearch.cpp
===================================================================
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -621,11 +621,12 @@
return CopyStr;
}
-static bool isFrameworkStylePath(StringRef Path,
+static bool isFrameworkStylePath(StringRef Path, bool &IsPrivateHeader,
SmallVectorImpl<char> &FrameworkName) {
using namespace llvm::sys;
path::const_iterator I = path::begin(Path);
path::const_iterator E = path::end(Path);
+ IsPrivateHeader = false;
// Detect different types of framework style paths:
//
@@ -637,12 +638,16 @@
// and some other variations among these lines.
int FoundComp = 0;
while (I != E) {
+ if (*I == "Headers")
+ ++FoundComp;
if (I->endswith(".framework")) {
FrameworkName.append(I->begin(), I->end());
++FoundComp;
}
- if (*I == "Headers" || *I == "PrivateHeaders")
+ if (*I == "PrivateHeaders") {
++FoundComp;
+ IsPrivateHeader = true;
+ }
++I;
}
@@ -654,11 +659,13 @@
StringRef Includer, StringRef IncludeFilename,
const FileEntry *IncludeFE, bool isAngled = false,
bool FoundByHeaderMap = false) {
+ bool IsIncluderPrivateHeader = false;
SmallString<128> FromFramework, ToFramework;
- if (!isFrameworkStylePath(Includer, FromFramework))
+ if (!isFrameworkStylePath(Includer, IsIncluderPrivateHeader, FromFramework))
return;
- bool IsIncludeeInFramework =
- isFrameworkStylePath(IncludeFE->getName(), ToFramework);
+ bool IsIncludeePrivateHeader = false;
+ bool IsIncludeeInFramework = isFrameworkStylePath(
+ IncludeFE->getName(), IsIncludeePrivateHeader, ToFramework);
if (!isAngled && !FoundByHeaderMap) {
SmallString<128> NewInclude("<");
@@ -672,6 +679,14 @@
<< IncludeFilename
<< FixItHint::CreateReplacement(IncludeLoc, NewInclude);
}
+
+ // Headers in Foo.framework/Headers should not include headers
+ // from Foo.framework/PrivateHeaders, since this violates public/private
+ // API boundaries and can cause modular dependency cycles.
+ if (!IsIncluderPrivateHeader && IsIncludeeInFramework &&
+ IsIncludeePrivateHeader && FromFramework == ToFramework)
+ Diags.Report(IncludeLoc, diag::warn_framework_include_private_from_public)
+ << IncludeFilename;
}
/// LookupFile - Given a "foo" or \<foo> reference, look up the indicated file,
Index: include/clang/Basic/DiagnosticLexKinds.td
===================================================================
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -718,6 +718,9 @@
"double-quoted include \"%0\" in framework header, "
"expected angle-bracketed instead"
>, InGroup<FrameworkHdrQuotedInclude>, DefaultIgnore;
+def warn_framework_include_private_from_public : Warning<
+ "public framework header includes private framework header '%0'"
+ >, InGroup<FrameworkIncludePrivateFromPublic>;
def warn_auto_module_import : Warning<
"treating #%select{include|import|include_next|__include_macros}0 as an "
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -32,6 +32,8 @@
def Section : DiagGroup<"section">;
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">;
def CXXPre14CompatBinaryLiteral : DiagGroup<"c++98-c++11-compat-binary-literal">;
def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
