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

Reply via email to