DmitryPolukhin created this revision.
DmitryPolukhin added reviewers: adamcz, sammccall, kadircet.
DmitryPolukhin added a project: clang.
Herald added subscribers: arphaman, javed.absar.
Herald added a project: All.
DmitryPolukhin requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
After https://reviews.llvm.org/D85753 these diagnostics silently dropped
so the user may not have an idea what is wrong. This diff moves
diagnostics to the start of the main file same as it happens with
errors in the command line. Also added prefix give to make it clear that the
location is not original.
Test Plan: check-clangd
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D158519
Files:
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/unittests/ModulesTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1079,7 +1079,8 @@
ElementsAre(AllOf(
Field(&Diag::ID, Eq(diag::err_drv_unknown_argument)),
Field(&Diag::Name, Eq("drv_unknown_argument")),
- Field(&Diag::Message, "unknown argument: '-fsome-unknown-flag'"))));
+ Field(&Diag::Message,
+ "in command line: unknown argument: '-fsome-unknown-flag'"))));
}
TEST_F(TUSchedulerTests, CommandLineWarnings) {
Index: clang-tools-extra/clangd/unittests/ModulesTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -18,6 +18,12 @@
namespace clangd {
namespace {
+using ::testing::ElementsAre;
+
+MATCHER_P(diag, Desc, "") {
+ return llvm::StringRef(arg.Message).contains(Desc);
+}
+
TEST(Modules, TextualIncludeInPreamble) {
TestTU TU = TestTU::withCode(R"cpp(
#include "Textual.h"
@@ -88,7 +94,13 @@
)modulemap";
// Test that we do not crash.
- TU.build();
+ auto AST = TU.build();
+
+ // Test expected diagnostics.
+ EXPECT_THAT(AST.getDiagnostics(),
+ ElementsAre(diag("in implicitly built module: module M does not "
+ "depend on a module exporting 'non-modular.h'"),
+ diag("could not build module 'M'")));
}
// Unknown module formats are a fatal failure for clang. Ensure we don't crash.
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -672,14 +672,14 @@
void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) {
- // If the diagnostic was generated for a different SourceManager, skip it.
+ bool FromModule = false;
+ // If the diagnostic was generated for a different SourceManager.
// This happens when a module is imported and needs to be implicitly built.
// The compilation of that module will use the same StoreDiags, but different
- // SourceManager.
+ // SourceManager. Treat them as coming from command line.
if (OrigSrcMgr && Info.hasSourceManager() &&
OrigSrcMgr != &Info.getSourceManager()) {
- IgnoreDiagnostics::log(DiagLevel, Info);
- return;
+ FromModule = true;
}
DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
@@ -687,7 +687,7 @@
Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
Info.getID());
- if (Info.getLocation().isInvalid()) {
+ if (Info.getLocation().isInvalid() || FromModule) {
// Handle diagnostics coming from command-line arguments. The source
manager
// is *not* available at this point, so we cannot use it.
if (!OriginallyError) {
@@ -702,6 +702,11 @@
LastDiagOriginallyError = OriginallyError;
LastDiag->ID = Info.getID();
fillNonLocationData(DiagLevel, Info, *LastDiag);
+ // Update message to mention original location.
+ const char *Prefix =
+ FromModule ? "in implicitly built module" : "in command line";
+ LastDiag->Message = llvm::formatv("{0}: {1}", Prefix, LastDiag->Message);
+
LastDiag->InsideMainFile = true;
// Put it at the start of the main file, for a lack of a better place.
LastDiag->Range.start = Position{0, 0};
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1079,7 +1079,8 @@
ElementsAre(AllOf(
Field(&Diag::ID, Eq(diag::err_drv_unknown_argument)),
Field(&Diag::Name, Eq("drv_unknown_argument")),
- Field(&Diag::Message, "unknown argument: '-fsome-unknown-flag'"))));
+ Field(&Diag::Message,
+ "in command line: unknown argument: '-fsome-unknown-flag'"))));
}
TEST_F(TUSchedulerTests, CommandLineWarnings) {
Index: clang-tools-extra/clangd/unittests/ModulesTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ModulesTests.cpp
+++ clang-tools-extra/clangd/unittests/ModulesTests.cpp
@@ -18,6 +18,12 @@
namespace clangd {
namespace {
+using ::testing::ElementsAre;
+
+MATCHER_P(diag, Desc, "") {
+ return llvm::StringRef(arg.Message).contains(Desc);
+}
+
TEST(Modules, TextualIncludeInPreamble) {
TestTU TU = TestTU::withCode(R"cpp(
#include "Textual.h"
@@ -88,7 +94,13 @@
)modulemap";
// Test that we do not crash.
- TU.build();
+ auto AST = TU.build();
+
+ // Test expected diagnostics.
+ EXPECT_THAT(AST.getDiagnostics(),
+ ElementsAre(diag("in implicitly built module: module M does not "
+ "depend on a module exporting 'non-modular.h'"),
+ diag("could not build module 'M'")));
}
// Unknown module formats are a fatal failure for clang. Ensure we don't crash.
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -672,14 +672,14 @@
void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) {
- // If the diagnostic was generated for a different SourceManager, skip it.
+ bool FromModule = false;
+ // If the diagnostic was generated for a different SourceManager.
// This happens when a module is imported and needs to be implicitly built.
// The compilation of that module will use the same StoreDiags, but different
- // SourceManager.
+ // SourceManager. Treat them as coming from command line.
if (OrigSrcMgr && Info.hasSourceManager() &&
OrigSrcMgr != &Info.getSourceManager()) {
- IgnoreDiagnostics::log(DiagLevel, Info);
- return;
+ FromModule = true;
}
DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
@@ -687,7 +687,7 @@
Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
Info.getID());
- if (Info.getLocation().isInvalid()) {
+ if (Info.getLocation().isInvalid() || FromModule) {
// Handle diagnostics coming from command-line arguments. The source manager
// is *not* available at this point, so we cannot use it.
if (!OriginallyError) {
@@ -702,6 +702,11 @@
LastDiagOriginallyError = OriginallyError;
LastDiag->ID = Info.getID();
fillNonLocationData(DiagLevel, Info, *LastDiag);
+ // Update message to mention original location.
+ const char *Prefix =
+ FromModule ? "in implicitly built module" : "in command line";
+ LastDiag->Message = llvm::formatv("{0}: {1}", Prefix, LastDiag->Message);
+
LastDiag->InsideMainFile = true;
// Put it at the start of the main file, for a lack of a better place.
LastDiag->Range.start = Position{0, 0};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
