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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/AST/ItaniumMangle.cpp:583
+    // Non-transparent overloadable functions need mangling.
+    if (auto *A = FD->getAttr<OverloadableAttr>())
+      if (!A->isTransparent())
----------------
`const auto *`


================
Comment at: lib/AST/MicrosoftMangle.cpp:372
+    // Non-transparent overloadable functions need mangling.
+    if (auto *A = FD->getAttr<OverloadableAttr>())
+      if (!A->isTransparent())
----------------
`const auto *`


================
Comment at: lib/Sema/SemaDecl.cpp:2820
+    for (const Decl *ND : Base->redecls())
+      if (auto *Ovl = ND->getAttr<OverloadableAttr>())
+        if (!Ovl->isImplicit())
----------------
`const auto *`


================
Comment at: lib/Sema/SemaDecl.cpp:2921
+      const auto *NewOvl = New->getAttr<OverloadableAttr>();
+      if (NewOvl->isTransparent() != OldOvl->isTransparent()) {
+        assert(!NewOvl->isImplicit() &&
----------------
Can `NewOvl` be null here (in an error case)?


================
Comment at: lib/Sema/SemaDecl.cpp:9303
+  } else if (!getLangOpts().CPlusPlus) {
+    if (auto *NewOvl = NewFD->getAttr<OverloadableAttr>()) {
+      if (NewOvl->isTransparent()) {
----------------
`const auto *`


================
Comment at: lib/Sema/SemaDecl.cpp:9306-9307
+        auto Transparent = llvm::find_if(Previous, [](const NamedDecl *ND) {
+          if (auto *FD = dyn_cast<FunctionDecl>(ND))
+            if (auto *Ovl =
+                    FD->getMostRecentDecl()->getAttr<OverloadableAttr>())
----------------
`const auto *` in both places.


================
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}}
+
----------------
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?


https://reviews.llvm.org/D32332