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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
