[PATCH] D59520: [WebAssembly] Address review comments on r352930

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:2594-2597
+  else if (const auto *IMA = dyn_cast<WebAssemblyImportModuleAttr>(Attr))
+    NewAttr = S.mergeImportModuleAttr(D, *IMA);
+  else if (const auto *INA = dyn_cast<WebAssemblyImportNameAttr>(Attr))
+    NewAttr = S.mergeImportNameAttr(D, *INA);
----------------
You should probably have some tests for the redeclaration behavior somewhere. The way you usually do this is to declare the function with the attribute, then declare the function again without the attribute but see that it is in fact inherited. We sometimes use AST dumping tests for this.


================
Comment at: clang/test/Sema/attr-wasm.c:11
+
+__attribute__((import_name("foo"))) void name_e() {} //FIXME-expected-error {{import name cannot be applied to a function with a definition}}
+
----------------
Are you intending to fix this as part of the patch?


================
Comment at: clang/test/Sema/attr-wasm.c:15
+
+void name_z() __attribute__((import_name("bar"))); //expected-warning {{import name does not match previous declaration}}
+
----------------
Uncertain how important you think this may be: should we include the import names in this diagnostic? e.g., `import name (%0) does not match the import name (%1) of the previous declaration` or somesuch? I don't know how often users will hide these attributes behind macros, which is the case I was worried might be confusing.


================
Comment at: clang/test/Sema/attr-wasm.c:25
+
+void module_e() __attribute__((import_module("foo"))) {} //FIXME-expected-error {{import module cannot be applied to a function with a definition}}
+
----------------
Same question about this FIXME.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59520/new/

https://reviews.llvm.org/D59520