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

Wed Mar 20 05:08:47 PDT 2019

aaron.ballman added a comment.

In D59520#1435728 <https://reviews.llvm.org/D59520#1435728>, @sunfish wrote:

> In D59520#1434854 <https://reviews.llvm.org/D59520#1434854>, @aaron.ballman wrote:
>
> > > Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.
> >
> > If you need to keep the prohibition that these attributes not be applied to functions with definitions, there are ways to accomplish that (we merge declarations and can decide what to do at that point if we see a declaration that is a definition). Given that, do you still want to lift this restriction?
>
>
> These attributes don't make sense on definitions. So right now, they are silently ignored when applied to definitions. That's effectively the current behavior too, because the existing check doesn't work as intended, so the change here doesn't change anything in practice yet.
>
> A diagnostic would be slightly tidier, but I'm not familiar enough with clang to do this quickly and didn't want to delay the rest of the patch while I investigated. Do you know the specific places we'd need to change to diagnose this?

I'd be fine addressing it in a follow-up patch, but silently ignoring attributes is something I consider a bug. Users have a *very* difficult time seeing the difference between "accepted and doing its thing" and "silently ignored" depending on the attribute semantics, so that's why we explicitly tell them when we're ignoring an attribute.

As for where to handle this, I'd follow our typical "merge" pattern for attributes. 1) Add `Sema::mergeImportNameAttr()` that does all your error checking for definitions and creates an attribute if there's no issues, 2) From `handleImportNameAttr()`, handle your typical semantic checks and then calls `S.mergeImportNameAttr()` to try to create the attribute, and if it is created, attach it to the declaration, 3) hook the new merge operation up in `mergeDeclAttribute()` in SemaDecl.cpp.

If you don't address it as part of this patch, can you add a bug to bugzilla so that we get this tracked (if you don't plan to fix the issue soon) and add a test case to this patch demonstrating that we are silently ignoring the attribute with a FIXME comment?

Repository:
  rC Clang

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

https://reviews.llvm.org/D59520