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

[Dan Gohman via Phabricator via cfe-commits]

sunfish marked 3 inline comments as done.
sunfish added a comment.

I apologize again for the major delay. I've now updated the patch and addressed all of your 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);
----------------
aaron.ballman wrote:
> 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.
I've now added tests for this as you suggested.


================
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}}
+
----------------
aaron.ballman wrote:
> Are you intending to fix this as part of the patch?
I've made a few attempts at fixing this, but I'm not very familiar with these parts of clang, and I've been unable to find another attribute with a similar diagnostic.

The `weak_import` attribute seems like it should be one, with its `warn_attribute_invalid_on_definition` warning, and  there's code in clang to issue that warning for both functions and variables, however the function version of the diagnostic doesn't seem to work, and only the variable version has tests.

So for now I've just removed these tests, to at least unblock the rest of this patch. If anyone knows how to implement these diagnostics, I'd be interested.


================
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}}
+
----------------
aaron.ballman wrote:
> 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.
I've now updated the patch to provide a more informative message, following your suggestion here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59520