aaron.ballman added inline comments.
================
Comment at: include/clang/Basic/DiagnosticIDs.h:39
DIAG_SIZE_CROSSTU = 100,
- DIAG_SIZE_SEMA = 3500,
+ DIAG_SIZE_SEMA = 3504,
DIAG_SIZE_ANALYSIS = 100,
----------------
I think this should be a separate commit, and I'd recommend updating it to `4000` to give ourselves more wiggle room.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9649-9656
+def warn_mismatched_import_module : Warning<
+ "import module does not match previous declaration">;
+def warn_mismatched_import_name : Warning<
+ "import name does not match previous declaration">;
+def warn_import_module_on_definition : Warning<
+ "import module cannot be applied to a function with a definition">;
+def warn_import_name_on_definition : Warning<
----------------
How about: `"import %select{module|name}0 does not match previous declaration"` and `"import %select{module|name}0 cannot be applied to a function with a definition"`?
================
Comment at: include/clang/Sema/Sema.h:2616-2621
+ WebAssemblyImportNameAttr *mergeImportNameAttr(
+ Decl *D, SourceRange Range, StringRef Name,
+ unsigned AttrSpellingListIndex);
+ WebAssemblyImportModuleAttr *mergeImportModuleAttr(
+ Decl *D, SourceRange Range, StringRef Name,
+ unsigned AttrSpellingListIndex);
----------------
I'd rather see the typical pattern used here: one taking a `ParsedAttr` and the other taking a semantic attribute.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5764-5765
+
+ if (WebAssemblyImportModuleAttr *ExistingAttr =
+ FD->getAttr<WebAssemblyImportModuleAttr>()) {
+ if (ExistingAttr->getImportModule() == Name)
----------------
`const auto *`
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5769
+ Diag(ExistingAttr->getLocation(), diag::warn_mismatched_import_module);
+ Diag(Range.getBegin(), diag::note_previous_attribute);
+ return nullptr;
----------------
I don't see anything testing this note or the preceding warning, can you add some tests that exercise it?
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5773
+ if (FD->hasBody()) {
+ Diag(Range.getBegin(), diag::warn_import_module_on_definition);
+ return nullptr;
----------------
I don't see any tests for this either.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5781-5783
+Sema::mergeImportNameAttr(Decl *D, SourceRange Range,
+ StringRef Name,
+ unsigned AttrSpellingListIndex) {
----------------
I wonder if we want to generalize this with a template (for the attribute type) if we could generalize the diagnostic text a bit more (or add an additional parameter for it)?
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5786-5787
+
+ if (WebAssemblyImportNameAttr *ExistingAttr =
+ FD->getAttr<WebAssemblyImportNameAttr>()) {
+ if (ExistingAttr->getImportName() == Name)
----------------
`const auto *`
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5790-5791
+ return nullptr;
+ Diag(ExistingAttr->getLocation(), diag::warn_mismatched_import_name);
+ Diag(Range.getBegin(), diag::note_previous_attribute);
+ return nullptr;
----------------
Missing tests.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5795
+ if (FD->hasBody()) {
+ Diag(Range.getBegin(), diag::warn_import_name_on_definition);
+ return nullptr;
----------------
Missing tests.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59520/new/
https://reviews.llvm.org/D59520