[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdc5cbba3196d: [clang][modules] Add
-Wsystem-headers-in-module= (authored by benlangmuir).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156948/new/
https://reviews.llvm.org/D156948
Files:
clang/include/clang/Basic/DiagnosticOptions.h
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
clang/test/ClangScanDeps/Wsystem-headers-in-module.c
clang/test/Modules/Wsystem-headers-in-module.c
Index: clang/test/Modules/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/Modules/Wsystem-headers-in-module.c
@@ -0,0 +1,32 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/mcp \
+// RUN: -isystem %t/sys %t/tu.c -fsyntax-only -Wextra-semi -Wsystem-headers-in-module=sys_mod \
+// RUN: 2>&1 | FileCheck %s
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- sys/other_sys_header.h
+int x;;
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;;
+//--- other_sys_mod.h
+int z;;
+//--- dependent_sys_mod.h
+int w;;
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
Index: clang/test/ClangScanDeps/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/Wsystem-headers-in-module.c
@@ -0,0 +1,56 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules when built with explicit
+// modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/deps.json
+
+// RUN: %deps-to-rsp %t/deps.json --module-name=dependent_sys_mod > %t/dependent_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=sys_mod > %t/sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=other_sys_mod > %t/other_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --tu-index 0 > %t/tu.rsp
+
+// RUN: %clang @%t/dependent_sys_mod.rsp -verify
+// RUN: %clang @%t/sys_mod.rsp -verify
+// RUN: %clang @%t/other_sys_mod.rsp -verify
+// RUN: %clang @%t/tu.rsp -verify
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- cdb.json.template
+[{
+ "directory": "DIR",
+ "command": "clang -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/mcp DIR/tu.c -isystem DIR/sys -Wextra-semi -Wsystem-headers-in-module=sys_mod",
+ "file": "DIR/tu.c"
+}]
+
+//--- sys/other_sys_header.h
+int x;;
+
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;; // expected-warning {{extra ';' outside of a function}}
+
+//--- other_sys_mod.h
+int z;;
+// expected-no-diagnostics
+
+//--- dependent_sys_mod.h
+int w;;
+// expected-no-diagnostics
+
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
+// expected-no-diagnostics
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -12,6 +12,7 @@
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/BLAKE3.h"
#include "llvm/Support/StringSaver.h"
#include
@@ -172,6 +173,13 @@
CI.getHeaderSearchOpts().ModulesIgnoreMacros.clear();
}
+ // Apply -Wsystem-headers-in-module for the current module.
+ if (llvm::is_contained(CI.getDiagnosticOpts().SystemHeaderWarningsModules,
+ Deps.ID.ModuleName))
+CI.getDiagnosticOpts().Warnings.push_back("system-headers");
+ // Remove the now unused option(s).
+ CI.getDiagnosticOpts().SystemHeaderWarningsModules.clear();
+
Optimize(CI);
return
[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=
benlangmuir marked an inline comment as done. benlangmuir added inline comments. Comment at: clang/include/clang/Basic/DiagnosticOptions.h:128 + /// whether -Wsystem-headers is enabled on a per-module basis. + std::vector SystemHeaderWarningsModules; + iana wrote: > benlangmuir wrote: > > jansvoboda11 wrote: > > > Out of interest, is there an existing use-case for having multiple > > > modules here, or is this just future-proofing? > > I'm not using it currently, but I think it makes sense to handle any > > number. You could have multiple such modules you're testing together. > I'd imagine you'd typically have Framework and Framework_Private in there Ah, good point. I am in fact doing that already and just forgot. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156948/new/ https://reviews.llvm.org/D156948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=
iana accepted this revision. iana added inline comments. Comment at: clang/include/clang/Basic/DiagnosticOptions.h:128 + /// whether -Wsystem-headers is enabled on a per-module basis. + std::vector SystemHeaderWarningsModules; + benlangmuir wrote: > jansvoboda11 wrote: > > Out of interest, is there an existing use-case for having multiple modules > > here, or is this just future-proofing? > I'm not using it currently, but I think it makes sense to handle any number. > You could have multiple such modules you're testing together. I'd imagine you'd typically have Framework and Framework_Private in there CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156948/new/ https://reviews.llvm.org/D156948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=
benlangmuir marked an inline comment as done. benlangmuir added inline comments. Comment at: clang/include/clang/Basic/DiagnosticOptions.h:128 + /// whether -Wsystem-headers is enabled on a per-module basis. + std::vector SystemHeaderWarningsModules; + jansvoboda11 wrote: > Out of interest, is there an existing use-case for having multiple modules > here, or is this just future-proofing? I'm not using it currently, but I think it makes sense to handle any number. You could have multiple such modules you're testing together. Comment at: clang/lib/Frontend/CompilerInstance.cpp:1238-1239 + for (StringRef Name : DiagOpts.SystemHeaderWarningsModules) +if (Name == ModuleName) + Instance.getDiagnostics().setSuppressSystemWarnings(false); jansvoboda11 wrote: > I assume `llvm::is_contained()` wouldn't be okay with the `std::string` and > `StringRef` mismatch? Nope, I just didn't think of it here for some reason. Updated. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156948/new/ https://reviews.llvm.org/D156948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=
benlangmuir updated this revision to Diff 547266.
benlangmuir added a comment.
Use `llvm::is_contained`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156948/new/
https://reviews.llvm.org/D156948
Files:
clang/include/clang/Basic/DiagnosticOptions.h
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
clang/test/ClangScanDeps/Wsystem-headers-in-module.c
clang/test/Modules/Wsystem-headers-in-module.c
Index: clang/test/Modules/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/Modules/Wsystem-headers-in-module.c
@@ -0,0 +1,32 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/mcp \
+// RUN: -isystem %t/sys %t/tu.c -fsyntax-only -Wextra-semi -Wsystem-headers-in-module=sys_mod \
+// RUN: 2>&1 | FileCheck %s
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- sys/other_sys_header.h
+int x;;
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;;
+//--- other_sys_mod.h
+int z;;
+//--- dependent_sys_mod.h
+int w;;
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
Index: clang/test/ClangScanDeps/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/Wsystem-headers-in-module.c
@@ -0,0 +1,56 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules when built with explicit
+// modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/deps.json
+
+// RUN: %deps-to-rsp %t/deps.json --module-name=dependent_sys_mod > %t/dependent_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=sys_mod > %t/sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=other_sys_mod > %t/other_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --tu-index 0 > %t/tu.rsp
+
+// RUN: %clang @%t/dependent_sys_mod.rsp -verify
+// RUN: %clang @%t/sys_mod.rsp -verify
+// RUN: %clang @%t/other_sys_mod.rsp -verify
+// RUN: %clang @%t/tu.rsp -verify
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- cdb.json.template
+[{
+ "directory": "DIR",
+ "command": "clang -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/mcp DIR/tu.c -isystem DIR/sys -Wextra-semi -Wsystem-headers-in-module=sys_mod",
+ "file": "DIR/tu.c"
+}]
+
+//--- sys/other_sys_header.h
+int x;;
+
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;; // expected-warning {{extra ';' outside of a function}}
+
+//--- other_sys_mod.h
+int z;;
+// expected-no-diagnostics
+
+//--- dependent_sys_mod.h
+int w;;
+// expected-no-diagnostics
+
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
+// expected-no-diagnostics
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -12,6 +12,7 @@
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/BLAKE3.h"
#include "llvm/Support/StringSaver.h"
#include
@@ -172,6 +173,13 @@
CI.getHeaderSearchOpts().ModulesIgnoreMacros.clear();
}
+ // Apply -Wsystem-headers-in-module for the current module.
+ if (llvm::is_contained(CI.getDiagnosticOpts().SystemHeaderWarningsModules,
+ Deps.ID.ModuleName))
+CI.getDiagnosticOpts().Warnings.push_back("system-headers");
+ // Remove the now unused option(s).
+ CI.getDiagnosticOpts().SystemHeaderWarningsModules.clear();
+
Optimize(CI);
return CI;
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++
[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=
jansvoboda11 accepted this revision. jansvoboda11 added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/include/clang/Basic/DiagnosticOptions.h:128 + /// whether -Wsystem-headers is enabled on a per-module basis. + std::vector SystemHeaderWarningsModules; + Out of interest, is there an existing use-case for having multiple modules here, or is this just future-proofing? Comment at: clang/lib/Frontend/CompilerInstance.cpp:1238-1239 + for (StringRef Name : DiagOpts.SystemHeaderWarningsModules) +if (Name == ModuleName) + Instance.getDiagnostics().setSuppressSystemWarnings(false); I assume `llvm::is_contained()` wouldn't be okay with the `std::string` and `StringRef` mismatch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156948/new/ https://reviews.llvm.org/D156948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156948: [clang][modules] Add -Wsystem-headers-in-module=
benlangmuir created this revision.
benlangmuir added reviewers: jansvoboda11, iana.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.
Add a way to enable -Wsystem-headers only for a specific module. This is useful
for validating a module that would otherwise not see system header diagnostics
without being flooded by diagnostics for unrelated headers/modules. It's
relatively common for a module to be marked [system] but still wish to validate
itself explicitly.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156948
Files:
clang/include/clang/Basic/DiagnosticOptions.h
clang/include/clang/Driver/Options.td
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
clang/test/ClangScanDeps/Wsystem-headers-in-module.c
clang/test/Modules/Wsystem-headers-in-module.c
Index: clang/test/Modules/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/Modules/Wsystem-headers-in-module.c
@@ -0,0 +1,32 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/mcp \
+// RUN: -isystem %t/sys %t/tu.c -fsyntax-only -Wextra-semi -Wsystem-headers-in-module=sys_mod \
+// RUN: 2>&1 | FileCheck %s
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- sys/other_sys_header.h
+int x;;
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;;
+//--- other_sys_mod.h
+int z;;
+//--- dependent_sys_mod.h
+int w;;
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
Index: clang/test/ClangScanDeps/Wsystem-headers-in-module.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/Wsystem-headers-in-module.c
@@ -0,0 +1,56 @@
+// Check that Wsystem-headers-in-module shows diagnostics in the named system
+// module, but not in other system headers or modules when built with explicit
+// modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/deps.json
+
+// RUN: %deps-to-rsp %t/deps.json --module-name=dependent_sys_mod > %t/dependent_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=sys_mod > %t/sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=other_sys_mod > %t/other_sys_mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --tu-index 0 > %t/tu.rsp
+
+// RUN: %clang @%t/dependent_sys_mod.rsp -verify
+// RUN: %clang @%t/sys_mod.rsp -verify
+// RUN: %clang @%t/other_sys_mod.rsp -verify
+// RUN: %clang @%t/tu.rsp -verify
+
+// CHECK-NOT: warning:
+// CHECK: sys_mod.h:2:7: warning: extra ';'
+// CHECK-NOT: warning:
+
+//--- cdb.json.template
+[{
+ "directory": "DIR",
+ "command": "clang -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/mcp DIR/tu.c -isystem DIR/sys -Wextra-semi -Wsystem-headers-in-module=sys_mod",
+ "file": "DIR/tu.c"
+}]
+
+//--- sys/other_sys_header.h
+int x;;
+
+//--- sys_mod.h
+#include "dependent_sys_mod.h"
+int y;; // expected-warning {{extra ';' outside of a function}}
+
+//--- other_sys_mod.h
+int z;;
+// expected-no-diagnostics
+
+//--- dependent_sys_mod.h
+int w;;
+// expected-no-diagnostics
+
+//--- module.modulemap
+module sys_mod [system] { header "sys_mod.h" }
+module other_sys_mod [system] { header "other_sys_mod.h" }
+module dependent_sys_mod [system] { header "dependent_sys_mod.h" }
+
+//--- tu.c
+#include "sys_mod.h"
+#include "other_sys_mod.h"
+#include "other_sys_header.h"
+// expected-no-diagnostics
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -172,6 +172,13 @@
CI.getHeaderSearchOpts().ModulesIgnoreMacros.clear();
}
+ // Apply -Wsystem-headers-in-module for the current module.
+ for (StringRef Name : CI.getDiagnosticOpts().SystemHeaderWarningsModules)
+if (Name == Deps.ID.ModuleName)
+ CI.getDiagnosticOpts().Warnings.push_back("system-headers");
+ // Remove the now unused option(s).
+ CI.getDiagnosticOpts().SystemHeaderWarningsModules.clear();
+
Optimize(CI);
return CI;
Index:
