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