[PATCH] D32332: Add support for transparent overloadable functions in clang

[George Burgess IV via Phabricator via cfe-commits]

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