george.burgess.iv added inline comments.
================
Comment at: lib/Sema/SemaDecl.cpp:2921
+ const auto *NewOvl = New->getAttr<OverloadableAttr>();
+ if (NewOvl->isTransparent() != OldOvl->isTransparent()) {
+ assert(!NewOvl->isImplicit() &&
----------------
aaron.ballman wrote:
> Can `NewOvl` be null here (in an error case)?
If it's null here, then the bug is in another piece of code.
In theory, after one `FunctionDecl` with some name `N` is tagged with
`overloadable`, all proceeding declarations/definition(s) with the name `N`
should be tagged with `overloadable`. We'll make implicit `OverloadableAttr`s
if the user fails to do this (`fixMissingOverloadableAttr`).
...Though, this code doesn't do a good job of calling that out. I tried adding
a note of this to `Sema::CheckFunctionDeclaration`; I dunno if there's a better
place to put it.
Regardless, added an assert here to clarify. :)
================
Comment at: test/Sema/overloadable.c:189
+ void to_foo5(int) __attribute__((overloadable)); // expected-note {{previous
overload}}
+ void to_foo5(int) __attribute__((transparently_overloadable)); //
expected-error{{mismatched transparency}}
+
----------------
aaron.ballman wrote:
> george.burgess.iv wrote:
> > aaron.ballman wrote:
> > > Why should this be a mismatch? Attributes can usually be added to
> > > redeclarations without an issue, and it's not unheard of for subsequent
> > > redeclarations to gain additional attributes. It seems like the behavior
> > > the user would expect from this is for the `transparently_overloadable`
> > > attribute to "win" and simply replaces, or silently augments, the
> > > `overloadable` attribute.
> > my issue with this was:
> >
> > ```
> > // foo.h
> > void foo(int) __attribute__((overloadable));
> >
> > // foo.c
> > void foo(int) __attribute__((overloadable)) {}
> >
> > // bar.c
> > #include "foo.h"
> >
> > void foo(int) __attribute__((transparently_overloadable));
> >
> > // calls to foo(int) now silently call @foo instead of the mangled version,
> > but only in this TU
> > ```
> >
> > though, i suppose this code going against our guidance of "overloads should
> > generally have internal linkage", and it's already possible to get yourself
> > in a similar situation today. so, as long as we don't allow `overloadable`
> > to "win" against `transparently_overloadable`, i'm OK with this.
> Hmm, I can see how your example might cause confusion for the user as well.
> Perhaps downgrading it from an error to a warning and maybe putting something
> in the docs about why that warning could lead to bad things would be a good
> approach?
Done.
I wasn't sure what you thought was best in the case of
```
void foo(int) __attribute__((overloadable));
void foo(int) __attribute__((transparently_overloadable));
void foo(int) __attribute__((overloadable));
```
so I made clang emit an error on the last redeclaration of `foo` because it's
not `transparently_overloadable`. If you'd prefer for us to silently turn the
last `overloadable` into `transparently_overloadable`, I'm happy to allow that,
too.
(In any case, we'll now warn about going from `overloadable` ->
`transparently_overloadable` on the middle decl)
https://reviews.llvm.org/D32332
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
