aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:9835-9839
+ if (D.isFunctionDefinition()) {
+ NewFD->setWillHaveBody();
+ ProcessDeclAttributes(S, NewFD, D);
+ NewFD->setWillHaveBody(false);
+ } else
----------------
sdesmalen wrote:
> aaron.ballman wrote:
> > This seems like a hack to work around what feels like a bug -- if the
> > declarator knows the function is a definition, then why does the
> > `FunctionDecl` AST node claim the function won't have a body? It seems
> > strange to me that we don't set that bit when acting on the function
> > declarator but instead wait until we're acting on the start of the function
> > definition to set it; does anything break if you start setting that flag
> > earlier?
> > This seems like a hack to work around what feels like a bug -- if the
> > declarator knows the function is a definition, then why does the
> > FunctionDecl AST node claim the function won't have a body?
> I agree it is a bit of a workaround, I wasn't sure if it was a bug or by
> design, but there are several places in the codebase that are invoked after
> this point that seem to depend on the fact that `willHaveBody` has not been
> set to determine if something is a redeclaration.
>
> For example, setting NewFD->setWillHaveBody() to `true` if
> `D.isFunctionDefinition()`, causes the following test to fail:
>
> ```template <class T1, class T2>
> struct pair {
> T1 first;
> T2 second;
>
> pair() : first(), second() {}
> pair(const T1 &a, const T2 &b) : first(a), second(b) {}
>
> template<class U1, class U2>
> pair(const pair<U1, U2> &other) : first(other.first),
> second(other.second) {}
> };
> ```
>
> I did some digging to see if I could fix this some other way, but didn't spot
> any easy ways to do this.
> I did some digging to see if I could fix this some other way, but didn't spot
> any easy ways to do this.
Yeah, it may require more involved changes, but I'm not super comfortable
extending our technical debt with this bit unless there's really no other
reasonable way to do it.
We don't have any attributes that behave the way you're proposing this one
behaves. Most often, attributes can be written on any declaration and are
inherited by subsequent declarations. Every once in a while we have an
attribute that can be written only on a declaration, but none that require a
function definition (some require a record definition though). So I'm not too
surprised this is an edge case we've not hit before.
Why is this limited to just the definition and not allowed on a declaration?
================
Comment at: clang/test/AST/ast-dump-wasm-attr-export.c:13-15
+__attribute__((export_name("export_red"))) void red(void);
+__attribute__((export_name("export_orange"))) void orange(void);
+__attribute__((export_name("export_yellow"))) void yellow(void);
----------------
This is either hiding a bug or materially changing the test.
1) It's hiding a bug because those functions are supposed to be diagnosed and
that's not happening:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L7313
2) If the checking code in SemaDeclAttr.cpp is actually unintentional for some
reason, then this materially changes the test coverage to demonstrate that the
attribute is not lost by a subsequent redeclaration without the attribute (so
when later code looks up a decl, it still sees the attribute).
I think it's most likely hiding a bug though.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128256/new/
https://reviews.llvm.org/D128256
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
