[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.
vsapsai marked an inline comment as done.
vsapsai added a comment.
Thanks for the review.
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:700
+ "no module named '%0' %select{found|in '%2'}1, "
+ "expected parent module to be defined before the submodule">;
def err_mmap_top_level_inferred_submodule : Error<
bruno wrote:
> vsapsai wrote:
> > Not sure about this second part of the message ("expected..."). Wanted to
> > make it more actionable but not entirely happy with the wording. Any
> > suggestions including removal are welcome.
> How about `, parent module must be defined before the submodule`?
Thanks, I think it is more direct and slightly more concise.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84458/new/
https://reviews.llvm.org/D84458
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8839e278ffca: [Modules] Improve error message when cannot
find parent module for submodule… (authored by vsapsai).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84458/new/
https://reviews.llvm.org/D84458
Files:
clang/include/clang/Basic/DiagnosticLexKinds.td
clang/lib/Lex/ModuleMap.cpp
clang/test/Modules/diagnostics.modulemap
Index: clang/test/Modules/diagnostics.modulemap
===
--- clang/test/Modules/diagnostics.modulemap
+++ clang/test/Modules/diagnostics.modulemap
@@ -28,3 +28,9 @@
header "quux.h" { size 1 mtime 2 }
header "no_attrs.h" {}
}
+
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:8: error: no module named
'unknown' found, parent module must be defined before the submodule
+module unknown.submodule {}
+module known_top_level {}
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:24: error: no module named
'unknown' in 'known_top_level', parent module must be defined before the
submodule
+module known_top_level.unknown.submodule {}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1903,18 +1903,16 @@
continue;
}
- if (ActiveModule) {
-Diags.Report(Id[I].second, diag::err_mmap_missing_module_qualified)
- << Id[I].first
- << ActiveModule->getTopLevelModule()->getFullModuleName();
- } else {
-Diags.Report(Id[I].second, diag::err_mmap_expected_module_name);
- }
+ Diags.Report(Id[I].second, diag::err_mmap_missing_parent_module)
+ << Id[I].first << (ActiveModule != nullptr)
+ << (ActiveModule
+ ? ActiveModule->getTopLevelModule()->getFullModuleName()
+ : "");
HadError = true;
- return;
}
-if (ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
+if (TopLevelModule &&
+ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
assert(ModuleMapFile != Map.getModuleMapFileForUniquing(TopLevelModule)
&&
"submodule defined in same file as 'module *' that allowed its "
"top-level module");
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
--- clang/include/clang/Basic/DiagnosticLexKinds.td
+++ clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -695,6 +695,9 @@
"no module named '%0' visible from '%1'">;
def err_mmap_missing_module_qualified : Error<
"no module named '%0' in '%1'">;
+def err_mmap_missing_parent_module: Error<
+ "no module named '%0' %select{found|in '%2'}1, "
+ "parent module must be defined before the submodule">;
def err_mmap_top_level_inferred_submodule : Error<
"only submodules and framework modules may be inferred with wildcard
syntax">;
def err_mmap_inferred_no_umbrella : Error<
Index: clang/test/Modules/diagnostics.modulemap
===
--- clang/test/Modules/diagnostics.modulemap
+++ clang/test/Modules/diagnostics.modulemap
@@ -28,3 +28,9 @@
header "quux.h" { size 1 mtime 2 }
header "no_attrs.h" {}
}
+
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:8: error: no module named 'unknown' found, parent module must be defined before the submodule
+module unknown.submodule {}
+module known_top_level {}
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:24: error: no module named 'unknown' in 'known_top_level', parent module must be defined before the submodule
+module known_top_level.unknown.submodule {}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1903,18 +1903,16 @@
continue;
}
- if (ActiveModule) {
-Diags.Report(Id[I].second, diag::err_mmap_missing_module_qualified)
- << Id[I].first
- << ActiveModule->getTopLevelModule()->getFullModuleName();
- } else {
-Diags.Report(Id[I].second, diag::err_mmap_expected_module_name);
- }
+ Diags.Report(Id[I].second, diag::err_mmap_missing_parent_module)
+ << Id[I].first << (ActiveModule != nullptr)
+ << (ActiveModule
+ ? ActiveModule->getTopLevelModule()->getFullModuleName()
+ : "");
HadError = true;
- return;
}
-if (ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
+if (TopLevelModule &&
+ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
assert(ModuleMapFile != Map.getModuleMapFileForUniquing(TopLevelModule) &&
"submodule defined in same file as 'module *' that allowed its "
"top-level module");
Index:
[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.
vsapsai updated this revision to Diff 287749.
vsapsai added a comment.
Tweak the error text.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84458/new/
https://reviews.llvm.org/D84458
Files:
clang/include/clang/Basic/DiagnosticLexKinds.td
clang/lib/Lex/ModuleMap.cpp
clang/test/Modules/diagnostics.modulemap
Index: clang/test/Modules/diagnostics.modulemap
===
--- clang/test/Modules/diagnostics.modulemap
+++ clang/test/Modules/diagnostics.modulemap
@@ -28,3 +28,9 @@
header "quux.h" { size 1 mtime 2 }
header "no_attrs.h" {}
}
+
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:8: error: no module named
'unknown' found, parent module must be defined before the submodule
+module unknown.submodule {}
+module known_top_level {}
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:24: error: no module named
'unknown' in 'known_top_level', parent module must be defined before the
submodule
+module known_top_level.unknown.submodule {}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1903,18 +1903,16 @@
continue;
}
- if (ActiveModule) {
-Diags.Report(Id[I].second, diag::err_mmap_missing_module_qualified)
- << Id[I].first
- << ActiveModule->getTopLevelModule()->getFullModuleName();
- } else {
-Diags.Report(Id[I].second, diag::err_mmap_expected_module_name);
- }
+ Diags.Report(Id[I].second, diag::err_mmap_missing_parent_module)
+ << Id[I].first << (ActiveModule != nullptr)
+ << (ActiveModule
+ ? ActiveModule->getTopLevelModule()->getFullModuleName()
+ : "");
HadError = true;
- return;
}
-if (ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
+if (TopLevelModule &&
+ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
assert(ModuleMapFile != Map.getModuleMapFileForUniquing(TopLevelModule)
&&
"submodule defined in same file as 'module *' that allowed its "
"top-level module");
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
--- clang/include/clang/Basic/DiagnosticLexKinds.td
+++ clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -695,6 +695,9 @@
"no module named '%0' visible from '%1'">;
def err_mmap_missing_module_qualified : Error<
"no module named '%0' in '%1'">;
+def err_mmap_missing_parent_module: Error<
+ "no module named '%0' %select{found|in '%2'}1, "
+ "parent module must be defined before the submodule">;
def err_mmap_top_level_inferred_submodule : Error<
"only submodules and framework modules may be inferred with wildcard
syntax">;
def err_mmap_inferred_no_umbrella : Error<
Index: clang/test/Modules/diagnostics.modulemap
===
--- clang/test/Modules/diagnostics.modulemap
+++ clang/test/Modules/diagnostics.modulemap
@@ -28,3 +28,9 @@
header "quux.h" { size 1 mtime 2 }
header "no_attrs.h" {}
}
+
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:8: error: no module named 'unknown' found, parent module must be defined before the submodule
+module unknown.submodule {}
+module known_top_level {}
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:24: error: no module named 'unknown' in 'known_top_level', parent module must be defined before the submodule
+module known_top_level.unknown.submodule {}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1903,18 +1903,16 @@
continue;
}
- if (ActiveModule) {
-Diags.Report(Id[I].second, diag::err_mmap_missing_module_qualified)
- << Id[I].first
- << ActiveModule->getTopLevelModule()->getFullModuleName();
- } else {
-Diags.Report(Id[I].second, diag::err_mmap_expected_module_name);
- }
+ Diags.Report(Id[I].second, diag::err_mmap_missing_parent_module)
+ << Id[I].first << (ActiveModule != nullptr)
+ << (ActiveModule
+ ? ActiveModule->getTopLevelModule()->getFullModuleName()
+ : "");
HadError = true;
- return;
}
-if (ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
+if (TopLevelModule &&
+ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
assert(ModuleMapFile != Map.getModuleMapFileForUniquing(TopLevelModule) &&
"submodule defined in same file as 'module *' that allowed its "
"top-level module");
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
---
[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.
bruno accepted this revision.
bruno added a comment.
This revision is now accepted and ready to land.
LGTM as is, minor suggestion below.
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:700
+ "no module named '%0' %select{found|in '%2'}1, "
+ "expected parent module to be defined before the submodule">;
def err_mmap_top_level_inferred_submodule : Error<
vsapsai wrote:
> Not sure about this second part of the message ("expected..."). Wanted to
> make it more actionable but not entirely happy with the wording. Any
> suggestions including removal are welcome.
How about `, parent module must be defined before the submodule`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84458/new/
https://reviews.llvm.org/D84458
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.
vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84458/new/ https://reviews.llvm.org/D84458 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.
vsapsai added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84458/new/ https://reviews.llvm.org/D84458 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.
vsapsai marked 3 inline comments as done.
vsapsai added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:700
+ "no module named '%0' %select{found|in '%2'}1, "
+ "expected parent module to be defined before the submodule">;
def err_mmap_top_level_inferred_submodule : Error<
Not sure about this second part of the message ("expected..."). Wanted to make
it more actionable but not entirely happy with the wording. Any suggestions
including removal are welcome.
Comment at: clang/lib/Lex/ModuleMap.cpp:1911
- } else {
-Diags.Report(Id[I].second, diag::err_mmap_expected_module_name);
- }
Both of `err_mmap_missing_module_qualified` and `err_mmap_expected_module_name`
are still used in other places.
Comment at: clang/lib/Lex/ModuleMap.cpp:1914
HadError = true;
- return;
}
I've removed this `return` because otherwise you get spurious "expected module
declaration" errors for valid module definition.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84458/new/
https://reviews.llvm.org/D84458
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D84458: [Modules] Improve error message when cannot find parent module for submodule definition.
vsapsai created this revision.
vsapsai added reviewers: rsmith, bruno, Bigcheese.
Herald added subscribers: ributzka, dexonsmith, jkorous.
Herald added a project: clang.
Before the change the diagnostic for
module unknown.submodule {}
was "error: expected module name" which is incorrect and misleading
because both "unknown" and "submodule" are valid module names.
We already have a better error message when a parent module is a
submodule itself and is missing. Make the error for a missing top-level
module more like the one for a submodule.
rdar://problem/64424407
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D84458
Files:
clang/include/clang/Basic/DiagnosticLexKinds.td
clang/lib/Lex/ModuleMap.cpp
clang/test/Modules/diagnostics.modulemap
Index: clang/test/Modules/diagnostics.modulemap
===
--- clang/test/Modules/diagnostics.modulemap
+++ clang/test/Modules/diagnostics.modulemap
@@ -28,3 +28,9 @@
header "quux.h" { size 1 mtime 2 }
header "no_attrs.h" {}
}
+
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:8: error: no module named
'unknown' found, expected parent module to be defined before the submodule
+module unknown.submodule {}
+module known_top_level {}
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:24: error: no module named
'unknown' in 'known_top_level', expected parent module to be defined before the
submodule
+module known_top_level.unknown.submodule {}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1903,18 +1903,16 @@
continue;
}
- if (ActiveModule) {
-Diags.Report(Id[I].second, diag::err_mmap_missing_module_qualified)
- << Id[I].first
- << ActiveModule->getTopLevelModule()->getFullModuleName();
- } else {
-Diags.Report(Id[I].second, diag::err_mmap_expected_module_name);
- }
+ Diags.Report(Id[I].second, diag::err_mmap_missing_parent_module)
+ << Id[I].first << (ActiveModule != nullptr)
+ << (ActiveModule
+ ? ActiveModule->getTopLevelModule()->getFullModuleName()
+ : "");
HadError = true;
- return;
}
-if (ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
+if (TopLevelModule &&
+ModuleMapFile != Map.getContainingModuleMapFile(TopLevelModule)) {
assert(ModuleMapFile != Map.getModuleMapFileForUniquing(TopLevelModule)
&&
"submodule defined in same file as 'module *' that allowed its "
"top-level module");
Index: clang/include/clang/Basic/DiagnosticLexKinds.td
===
--- clang/include/clang/Basic/DiagnosticLexKinds.td
+++ clang/include/clang/Basic/DiagnosticLexKinds.td
@@ -695,6 +695,9 @@
"no module named '%0' visible from '%1'">;
def err_mmap_missing_module_qualified : Error<
"no module named '%0' in '%1'">;
+def err_mmap_missing_parent_module: Error<
+ "no module named '%0' %select{found|in '%2'}1, "
+ "expected parent module to be defined before the submodule">;
def err_mmap_top_level_inferred_submodule : Error<
"only submodules and framework modules may be inferred with wildcard
syntax">;
def err_mmap_inferred_no_umbrella : Error<
Index: clang/test/Modules/diagnostics.modulemap
===
--- clang/test/Modules/diagnostics.modulemap
+++ clang/test/Modules/diagnostics.modulemap
@@ -28,3 +28,9 @@
header "quux.h" { size 1 mtime 2 }
header "no_attrs.h" {}
}
+
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:8: error: no module named 'unknown' found, expected parent module to be defined before the submodule
+module unknown.submodule {}
+module known_top_level {}
+// CHECK: diagnostics.modulemap:[[@LINE+1]]:24: error: no module named 'unknown' in 'known_top_level', expected parent module to be defined before the submodule
+module known_top_level.unknown.submodule {}
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1903,18 +1903,16 @@
continue;
}
- if (ActiveModule) {
-Diags.Report(Id[I].second, diag::err_mmap_missing_module_qualified)
- << Id[I].first
- << ActiveModule->getTopLevelModule()->getFullModuleName();
- } else {
-Diags.Report(Id[I].second, diag::err_mmap_expected_module_name);
- }
+ Diags.Report(Id[I].second, diag::err_mmap_missing_parent_module)
+ << Id[I].first << (ActiveModule != nullptr)
+ << (ActiveModule
+ ? ActiveModule->getTopLevelModule()->getFullModuleName()
+ : "");
HadError = true;
- return;
}
-if (ModuleMapFile !=
