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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

thanks for the feedback!

fwiw, at a high level, the main problem i'm trying to solve with this is that you can't really make a `__overloadable_without_mangling` macro without handing it the function name, since you currently need to use `__asm__("name-you'd-like-the-function-to-have")` to rename the function. if you can think of a better way to go about this, even if it requires that we drop the "no `overloadable` required on some redeclarations" feature this adds, i'm all ears. :)

> Is there a reason we want the second spelling instead of making the current spelling behave in the manner you describe?

there are two features this is adding, and i'm not sure which you want `overloadable` to imply.

1. this new spelling brings with it the promise that we won't mangle function names; if we made every `overloadable` act this way, we'd have:

  void foo(int) __attribute__((overloadable)); // emitted as `declare void @foo(i32)`
  void foo(long) __attribute__((overloadable)); // emitted as `declare void @foo(i64)`. LLVM gets angry, since we have different declarations for @foo.

the only way i can think of to get "no indication of which overloads aren't mangled" to work is choosing which overload to mangle based on source order, and that seems overly subtle to me. so, i think this behavior needs to be spelled out in the source.

2. for the "no overloadable required on redecls" feature this adds: i wasn't there for the initial design of the `overloadable` attribute, so i can't say for certain, but i think part of the reason that we required `overloadable` to be on every redecl was so that people would have an indication of "hey, by the way, this name will probably be mangled and there may be other functions you end up calling." in C, i can totally see that being valuable. if you're suggesting we relax that for all overloads, i'll find who originally implemented all of this to figure out what their reasoning was and get back to you. :)

---

in any case, whether we actually do this as a new spelling, an optional argument to `overloadable`, etc. makes no difference to me. i chose a new spelling because AFAICT, we don't currently have a standard way for a user to query clang for if it has support for specific features in an attribute. (outside of checking clang's version, but that's discouraged AFAIK.) if we decide an optional arg would be a better approach than a new spelling, i'm happy to add something like `__has_attribute_ext` so you can do `__has_attribute_ext(overloadable, transparent_overloads)`.

>   I think that forcing the user to choice which spelling of "overloadable" they want to use is not very user friendly.

yeah. the idea was that users should only need to reach for `transparently_overloadable` if they're trying to make a previously non-`overloadable` function `overloadable`. like said, if you think there's a better way to surface this functionality, i'm all for it.



================
Comment at: include/clang/Basic/Attr.td:1416
 def Overloadable : Attr {
-  let Spellings = [GNU<"overloadable">];
+  let Spellings = [GNU<"overloadable">, GNU<"transparently_overloadable">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
----------------
aaron.ballman wrote:
> I'm not too keen on the name, especially since there's already a "transparent_union" attribute, where the "transparent" means something slightly different than it does here. However, my attempts to dream up a better name are coming up short...
yeah :/

my only other idea was `implicitly_overloadable`. maybe that would be preferable? 


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3259
   "'overloadable' function %0 must have a prototype">;
+def err_attribute_overloadable_too_many_transparent_overloads : Error<
+  "only one 'overloadable' overload may be transparent">;
----------------
aaron.ballman wrote:
> Why should this be an error instead of simply an ignored attribute?
if the user asks for us to not mangle two overloads with the same name, i don't think we can (sanely) go to CodeGen. see below.


================
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:
> 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.


https://reviews.llvm.org/D32332