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

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL306467: [Sema] Allow unmarked overloadable functions. 
(authored by gbiv).

Changed prior to commit:
  https://reviews.llvm.org/D32332?vs=103448=104277#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D32332

Files:
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Lex/PPMacroExpansion.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/test/CodeGen/mangle-ms.c
  cfe/trunk/test/CodeGen/mangle.c
  cfe/trunk/test/CodeGenCXX/mangle-ms.cpp
  cfe/trunk/test/PCH/attrs.c
  cfe/trunk/test/Sema/overloadable.c

Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3294,13 +3294,15 @@
   "IBOutletCollection properties should be copy/strong and not assign">,
   InGroup;
   
-def err_attribute_overloadable_missing : Error<
-  "%select{overloaded function|redeclaration of}0 %1 must have the "
-  "'overloadable' attribute">;
+def err_attribute_overloadable_mismatch : Error<
+  "redeclaration of %0 must %select{not |}1have the 'overloadable' attribute">;
 def note_attribute_overloadable_prev_overload : Note<
-  "previous overload of function is here">;
+  "previous %select{unmarked |}0overload of function is here">;
 def err_attribute_overloadable_no_prototype : Error<
   "'overloadable' function %0 must have a prototype">;
+def err_attribute_overloadable_multiple_unmarked_overloads : Error<
+  "at most one overload for a given name may lack the 'overloadable' "
+  "attribute">;
 def warn_ns_attribute_wrong_return_type : Warning<
   "%0 attribute only applies to %select{functions|methods|properties}1 that "
   "return %select{an Objective-C object|a pointer|a non-retainable pointer}2">,
Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -605,20 +605,27 @@
   for ``T`` and ``U`` to be incompatible.
 
 The declaration of ``overloadable`` functions is restricted to function
-declarations and definitions.  Most importantly, if any function with a given
-name is given the ``overloadable`` attribute, then all function declarations
-and definitions with that name (and in that scope) must have the
-``overloadable`` attribute.  This rule even applies to redeclarations of
-functions whose original declaration had the ``overloadable`` attribute, e.g.,
+declarations and definitions.  If a function is marked with the ``overloadable``
+attribute, then all declarations and definitions of functions with that name,
+except for at most one (see the note below about unmarked overloads), must have
+the ``overloadable`` attribute.  In addition, redeclarations of a function with
+the ``overloadable`` attribute must have the ``overloadable`` attribute, and
+redeclarations of a function without the ``overloadable`` attribute must *not*
+have the ``overloadable`` attribute. e.g.,
 
 .. code-block:: c
 
   int f(int) __attribute__((overloadable));
   float f(float); // error: declaration of "f" must have the "overloadable" attribute
+  int f(int); // error: redeclaration of "f" must have the "overloadable" attribute
 
   int g(int) __attribute__((overloadable));
   int g(int) { } // error: redeclaration of "g" must also have the "overloadable" attribute
 
+  int h(int);
+  int h(int) __attribute__((overloadable)); // error: declaration of "h" must not
+// have the "overloadable" attribute
+
 Functions marked ``overloadable`` must have prototypes.  Therefore, the
 following code is ill-formed:
 
@@ -651,7 +658,28 @@
   linkage specification, it's name *will* be mangled in the same way as it
   would in C.
 
-Query for this feature with ``__has_extension(attribute_overloadable)``.
+For the purpose of backwards compatibility, at most one function with the same
+name as other ``overloadable`` functions may omit the ``overloadable``
+attribute. In this case, the function without the ``overloadable`` attribute
+will not have its name mangled.
+
+For example:
+
+.. code-block:: c
+
+  // Notes with mangled names assume Itanium mangling.
+  int f(int);
+  int f(double) __attribute__((overloadable));
+  void foo() {
+f(5); // Emits a call to f (not _Z1fi, as it would with an overload that
+  // was marked with overloadable).
+f(1.0); // Emits a call to _Z1fd.
+  }
+
+Support for unmarked overloads is not present in some versions of clang. You may
+query for it using ``__has_extension(overloadable_unmarked)``.
+
+Query for this attribute with ``__has_attribute(overloadable)``.
   }];
 }
 
Index: cfe/trunk/test/CodeGenCXX/mangle-ms.cpp

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Woohoo! Thanks again to both of you.


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thank you both for the comments!




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3294-3295
+def err_attribute_overloadable_multiple_unmarked_overloads : Error<
+  "at most one 'overloadable' function for a given name may lack the "
+  "'overloadable' attribute">;
 def warn_ns_attribute_wrong_return_type : Warning<

rsmith wrote:
> I think calling the functions 'overloadable' in this case is confusing.
Yeah, I couldn't think of a good name, either. Is this better?



Comment at: lib/Lex/PPMacroExpansion.cpp:1136
   .Case("nullability_on_arrays", true)
+  .Case("overloadable_unmarked", true)
   .Case("memory_sanitizer", LangOpts.Sanitize.has(SanitizerKind::Memory))

rsmith wrote:
> Should this be `__has_extension` rather than `__has_feature`, since it's not 
> a standard feature? (Per 
> http://clang.llvm.org/docs/LanguageExtensions.html#has-feature-and-has-extension
>  `__has_feature` is only for detecting standard features, despite us using it 
> for other things.)
Ah, I missed the "For backward compatibility" bit. Good call



Comment at: lib/Sema/SemaDecl.cpp:9225
+/*NewIsUsingDecl=*/false)) {
+  case Sema::Ovl_Match:
+  case Sema::Ovl_NonFunction:

aaron.ballman wrote:
> Why add `Sema::` to these?
Artifact of me moving code around and then putting it back. :)



Comment at: lib/Sema/SemaDecl.cpp:9249-9252
+case Ovl_Match:
+  // We're not guaranteed to be handed the most recent decl, and
+  // __attribute__((overloadable)) depends somewhat on source order.
+  OldDecl = OldDecl->getMostRecentDecl();

rsmith wrote:
> `checkForConflictWithnonVisibleExternC` should probably be responsible for 
> finding the most recent declaration of the entity that is actually visible 
> (and not, say, a block-scope declaration in some unrelated function, or a 
> declaration in an unimported module).
Isn't part of the goal of `checkForConflictWithNonVisibleExternC` to report 
things that (potentially) aren't visible? If so, I don't see how it can also be 
its responsibility to always report things that are visible, unless it's a 
best-effort sort of thing.

In any case, this bit of code boils down to me trying to slip a bugfix in here. 
Since I think fixing this properly will just pollute this diff even more, I'll 
back it out and try again in a separate patch. :)



Comment at: lib/Sema/SemaDecl.cpp:9375-9381
+assert((Previous.empty() ||
+llvm::any_of(
+Previous,
+[](const NamedDecl *ND) {
+  return ND->getMostRecentDecl()->hasAttr();
+})) &&
+   "Non-redecls shouldn't happen without overloadable present");

rsmith wrote:
> Seems worth factoring out this "contains any overloadable functions" check, 
> since it's quite a few lines and you're doing it in two different places.
With the removal of one unnecessary change (see a later response), this is no 
longer repeated. Happy to still pull it out into a function if you think that 
helps readability.



Comment at: test/Sema/overloadable.c:112
   void (*ptr2)(char *) = 
-  void (*ambiguous)(int *) =  // expected-error{{initializing 'void 
(*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate 
function}}
-  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an 
expression of incompatible type ''}} 
expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) =  // expected-error{{initializing 'void 
(*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate 
function}}
+  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an 
expression of incompatible type ''}} 
expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}

rsmith wrote:
> It'd be nice to check in these changes from @NNN -> @-M separately.
r305947.


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 103448.
george.burgess.iv marked 6 inline comments as done.
george.burgess.iv added a comment.

Addressed all feedback


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/PCH/attrs.c
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,12 +3,15 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
-int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
-float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
+int *f(int) __attribute__((overloadable)); // expected-note{{previous overload of function is here}}
+float *f(float);
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
  // expected-note{{previous declaration is here}}
 double *f(double) __attribute__((overloadable)); // okay, new
 
+// Ensure we don't complain about overloadable on implicitly declared functions.
+int isdigit(int) __attribute__((overloadable));
+
 void test_f(int iv, float fv, double dv) {
   int *ip = f(iv);
   float *fp = f(fv);
@@ -71,19 +74,19 @@
   f1();
 }
 
-void before_local_1(int) __attribute__((overloadable)); // expected-note {{here}}
+void before_local_1(int) __attribute__((overloadable));
 void before_local_2(int); // expected-note {{here}}
 void before_local_3(int) __attribute__((overloadable));
 void local() {
-  void before_local_1(char); // expected-error {{must have the 'overloadable' attribute}}
-  void before_local_2(char) __attribute__((overloadable)); // expected-error {{conflicting types}}
+  void before_local_1(char);
+  void before_local_2(char); // expected-error {{conflicting types}}
   void before_local_3(char) __attribute__((overloadable));
-  void after_local_1(char); // expected-note {{here}}
-  void after_local_2(char) __attribute__((overloadable)); // expected-note {{here}}
+  void after_local_1(char);
+  void after_local_2(char) __attribute__((overloadable));
   void after_local_3(char) __attribute__((overloadable));
 }
-void after_local_1(int) __attribute__((overloadable)); // expected-error {{conflicting types}}
-void after_local_2(int); // expected-error {{must have the 'overloadable' attribute}}
+void after_local_1(int) __attribute__((overloadable));
+void after_local_2(int);
 void after_local_3(int) __attribute__((overloadable));
 
 // Make sure we allow C-specific conversions in C.
@@ -152,6 +155,85 @@
   foo(vcharbuf); // expected-error{{call to 'foo' is ambiguous}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
 }
 
+void overloadable_with_global() {
+  void wg_foo(void) __attribute__((overloadable)); // expected-note{{previous}}
+  void wg_foo(int) __attribute__((overloadable));
+}
+
+int wg_foo; // expected-error{{redefinition of 'wg_foo' as different kind of symbol}}
+
+#if !__has_extension(overloadable_unmarked)
+#error "We should have unmarked overload support"
+#endif
+
+void to_foo0(int);
+void to_foo0(double) __attribute__((overloadable)); // expected-note{{previous overload}}
+void to_foo0(int);
+void to_foo0(double); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo0(int);
+
+void to_foo1(int) __attribute__((overloadable)); // expected-note 2{{previous overload}}
+void to_foo1(double);
+void to_foo1(int); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo1(double);
+void to_foo1(int); // expected-error{{must have the 'overloadable' attribute}}
+
+void to_foo2(int); // expected-note{{previous unmarked overload}}
+void to_foo2(double) __attribute__((overloadable)); // expected-note 2{{previous overload}}
+void to_foo2(int) __attribute__((overloadable)); // expected-error {{must not have the 'overloadable' attribute}}
+void to_foo2(double); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo2(int);
+void to_foo2(double); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo2(int);
+
+void to_foo3(int);
+void to_foo3(double) __attribute__((overloadable)); // expected-note{{previous overload}}
+void to_foo3(int);
+void to_foo3(double); // expected-error{{must have the 'overloadable' attribute}}
+
+void to_foo4(int) __attribute__((overloadable)); // expected-note{{previous overload}}
+void to_foo4(int); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo4(double) __attribute__((overloadable));
+
+void to_foo5(int);
+void 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/Basic/AttrDocs.td:608-611
 declarations and definitions.  Most importantly, if any function with a given
 name is given the ``overloadable`` attribute, then all function declarations
 and definitions with that name (and in that scope) must have the
 ``overloadable`` attribute.  This rule even applies to redeclarations of

Can you update this to point out the exception below?



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3294-3295
+def err_attribute_overloadable_multiple_unmarked_overloads : Error<
+  "at most one 'overloadable' function for a given name may lack the "
+  "'overloadable' attribute">;
 def warn_ns_attribute_wrong_return_type : Warning<

I think calling the functions 'overloadable' in this case is confusing.



Comment at: lib/Lex/PPMacroExpansion.cpp:1136
   .Case("nullability_on_arrays", true)
+  .Case("overloadable_unmarked", true)
   .Case("memory_sanitizer", LangOpts.Sanitize.has(SanitizerKind::Memory))

Should this be `__has_extension` rather than `__has_feature`, since it's not a 
standard feature? (Per 
http://clang.llvm.org/docs/LanguageExtensions.html#has-feature-and-has-extension
 `__has_feature` is only for detecting standard features, despite us using it 
for other things.)



Comment at: lib/Sema/SemaDecl.cpp:9249-9252
+case Ovl_Match:
+  // We're not guaranteed to be handed the most recent decl, and
+  // __attribute__((overloadable)) depends somewhat on source order.
+  OldDecl = OldDecl->getMostRecentDecl();

`checkForConflictWithnonVisibleExternC` should probably be responsible for 
finding the most recent declaration of the entity that is actually visible (and 
not, say, a block-scope declaration in some unrelated function, or a 
declaration in an unimported module).



Comment at: lib/Sema/SemaDecl.cpp:9375-9381
+assert((Previous.empty() ||
+llvm::any_of(
+Previous,
+[](const NamedDecl *ND) {
+  return ND->getMostRecentDecl()->hasAttr();
+})) &&
+   "Non-redecls shouldn't happen without overloadable present");

Seems worth factoring out this "contains any overloadable functions" check, 
since it's quite a few lines and you're doing it in two different places.



Comment at: test/Sema/overloadable.c:112
   void (*ptr2)(char *) = 
-  void (*ambiguous)(int *) =  // expected-error{{initializing 'void 
(*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate 
function}}
-  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an 
expression of incompatible type ''}} 
expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) =  // expected-error{{initializing 'void 
(*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate 
function}}
+  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an 
expression of incompatible type ''}} 
expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}

It'd be nice to check in these changes from @NNN -> @-M separately.


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 102850.
george.burgess.iv marked 3 inline comments as done.
george.burgess.iv added a comment.

Addressed all feedback


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/PCH/attrs.c
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,12 +3,15 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
-int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
-float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
+int *f(int) __attribute__((overloadable)); // expected-note{{previous overload of function is here}}
+float *f(float);
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
  // expected-note{{previous declaration is here}}
 double *f(double) __attribute__((overloadable)); // okay, new
 
+// Ensure we don't complain about overloadable on implicitly declared functions.
+int isdigit(int) __attribute__((overloadable));
+
 void test_f(int iv, float fv, double dv) {
   int *ip = f(iv);
   float *fp = f(fv);
@@ -71,19 +74,19 @@
   f1();
 }
 
-void before_local_1(int) __attribute__((overloadable)); // expected-note {{here}}
+void before_local_1(int) __attribute__((overloadable));
 void before_local_2(int); // expected-note {{here}}
 void before_local_3(int) __attribute__((overloadable));
 void local() {
-  void before_local_1(char); // expected-error {{must have the 'overloadable' attribute}}
-  void before_local_2(char) __attribute__((overloadable)); // expected-error {{conflicting types}}
+  void before_local_1(char);
+  void before_local_2(char); // expected-error {{conflicting types}}
   void before_local_3(char) __attribute__((overloadable));
-  void after_local_1(char); // expected-note {{here}}
-  void after_local_2(char) __attribute__((overloadable)); // expected-note {{here}}
+  void after_local_1(char);
+  void after_local_2(char) __attribute__((overloadable));
   void after_local_3(char) __attribute__((overloadable));
 }
-void after_local_1(int) __attribute__((overloadable)); // expected-error {{conflicting types}}
-void after_local_2(int); // expected-error {{must have the 'overloadable' attribute}}
+void after_local_1(int) __attribute__((overloadable));
+void after_local_2(int);
 void after_local_3(int) __attribute__((overloadable));
 
 // Make sure we allow C-specific conversions in C.
@@ -106,8 +109,8 @@
   void foo(char *c) __attribute__((overloadable));
   void (*ptr1)(void *) = 
   void (*ptr2)(char *) = 
-  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
-  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
+  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}
 
   void (*specific1)(int *) = (void (*)(void *)) // expected-warning{{incompatible function pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
   void *specific2 = (void (*)(void *))
@@ -117,8 +120,8 @@
   void disabled(char *c) __attribute__((overloadable, enable_if(1, "The function name lies.")));
   // To be clear, these should all point to the last overload of 'disabled'
   void (*dptr1)(char *c) = 
-  void (*dptr2)(void *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
-  void (*dptr3)(int *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is generally looking good to me, with a few small nits. @rsmith, do you 
have thought on this?




Comment at: lib/Sema/SemaDecl.cpp:2875-2876
 /// Returns true if there was an error, false otherwise.
-bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *,
- Scope *S, bool MergeTypeWithOld) {
+bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *, Scope *S,
+ bool MergeTypeWithOld) {
   // Verify the old decl was also a function.

Seems to be a formatting-only change that's unrelated to the patch; can be 
reverted.



Comment at: lib/Sema/SemaDecl.cpp:9225
+/*NewIsUsingDecl=*/false)) {
+  case Sema::Ovl_Match:
+  case Sema::Ovl_NonFunction:

Why add `Sema::` to these?



Comment at: lib/Sema/SemaDecl.cpp:9383
+
+auto OtherUnmarkedIter = llvm::find_if(Previous, [&](const NamedDecl *ND) {
+  const auto *FD = dyn_cast(ND);

Why does this need a capture?


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 102207.
george.burgess.iv added a comment.

Swap to using `__has_feature(overloadable_unmarked)` for detection, as 
recommended by Hal in https://reviews.llvm.org/D33904


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/PCH/attrs.c
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,12 +3,15 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
-int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
-float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
+int *f(int) __attribute__((overloadable)); // expected-note{{previous overload of function is here}}
+float *f(float);
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
  // expected-note{{previous declaration is here}}
 double *f(double) __attribute__((overloadable)); // okay, new
 
+// Ensure we don't complain about overloadable on implicitly declared functions.
+int isdigit(int) __attribute__((overloadable));
+
 void test_f(int iv, float fv, double dv) {
   int *ip = f(iv);
   float *fp = f(fv);
@@ -71,19 +74,19 @@
   f1();
 }
 
-void before_local_1(int) __attribute__((overloadable)); // expected-note {{here}}
+void before_local_1(int) __attribute__((overloadable));
 void before_local_2(int); // expected-note {{here}}
 void before_local_3(int) __attribute__((overloadable));
 void local() {
-  void before_local_1(char); // expected-error {{must have the 'overloadable' attribute}}
-  void before_local_2(char) __attribute__((overloadable)); // expected-error {{conflicting types}}
+  void before_local_1(char);
+  void before_local_2(char); // expected-error {{conflicting types}}
   void before_local_3(char) __attribute__((overloadable));
-  void after_local_1(char); // expected-note {{here}}
-  void after_local_2(char) __attribute__((overloadable)); // expected-note {{here}}
+  void after_local_1(char);
+  void after_local_2(char) __attribute__((overloadable));
   void after_local_3(char) __attribute__((overloadable));
 }
-void after_local_1(int) __attribute__((overloadable)); // expected-error {{conflicting types}}
-void after_local_2(int); // expected-error {{must have the 'overloadable' attribute}}
+void after_local_1(int) __attribute__((overloadable));
+void after_local_2(int);
 void after_local_3(int) __attribute__((overloadable));
 
 // Make sure we allow C-specific conversions in C.
@@ -106,8 +109,8 @@
   void foo(char *c) __attribute__((overloadable));
   void (*ptr1)(void *) = 
   void (*ptr2)(char *) = 
-  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
-  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
+  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}
 
   void (*specific1)(int *) = (void (*)(void *)) // expected-warning{{incompatible function pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
   void *specific2 = (void (*)(void *))
@@ -117,8 +120,8 @@
   void disabled(char *c) __attribute__((overloadable, enable_if(1, "The function name lies.")));
   // To be clear, these should all point to the last overload of 'disabled'
   void (*dptr1)(char *c) = 
-  void (*dptr2)(void *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
-  void (*dptr3)(int *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible 

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Ping :)

https://reviews.llvm.org/D33904 is what I imagine would be a good way to detect 
this change.


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 100201.
george.burgess.iv added a comment.
Herald added a subscriber: jfb.

Fix the aforementioned issue; PTAL.

Note that this fix is slightly backwards incompatible. It disallows code like:

  void foo(int);
  void foo(int) __attribute__((overloadable)); // first decl lacked 
overloadable, so this one must lack it, as well.

I chose to do it this way because r64414 wanted to make us reject code like:

  double sin(double) __attribute__((overloadable));
  #include  // error: sin redecl without overloadable

...but it was trivial to get around that by moving the overloadable `sin` decl 
below the include. So, I feel like us accepting the first code snippet is a 
small bug.

I tested this change on ChromeOS, which uses `overloadable` in some parts of 
its C standard library. This backwards incompatibility caused no build 
breakages on that platform.

If we'd rather stay as backwards-compatible as possible, I have no issues with 
tweaking this to allow the first example.


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/PCH/attrs.c
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,12 +3,15 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
-int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
-float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
+int *f(int) __attribute__((overloadable)); // expected-note{{previous overload of function is here}}
+float *f(float);
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
  // expected-note{{previous declaration is here}}
 double *f(double) __attribute__((overloadable)); // okay, new
 
+// Ensure we don't complain about overloadable on implicitly declared functions.
+int isdigit(int) __attribute__((overloadable));
+
 void test_f(int iv, float fv, double dv) {
   int *ip = f(iv);
   float *fp = f(fv);
@@ -71,19 +74,19 @@
   f1();
 }
 
-void before_local_1(int) __attribute__((overloadable)); // expected-note {{here}}
+void before_local_1(int) __attribute__((overloadable));
 void before_local_2(int); // expected-note {{here}}
 void before_local_3(int) __attribute__((overloadable));
 void local() {
-  void before_local_1(char); // expected-error {{must have the 'overloadable' attribute}}
-  void before_local_2(char) __attribute__((overloadable)); // expected-error {{conflicting types}}
+  void before_local_1(char);
+  void before_local_2(char); // expected-error {{conflicting types}}
   void before_local_3(char) __attribute__((overloadable));
-  void after_local_1(char); // expected-note {{here}}
-  void after_local_2(char) __attribute__((overloadable)); // expected-note {{here}}
+  void after_local_1(char);
+  void after_local_2(char) __attribute__((overloadable));
   void after_local_3(char) __attribute__((overloadable));
 }
-void after_local_1(int) __attribute__((overloadable)); // expected-error {{conflicting types}}
-void after_local_2(int); // expected-error {{must have the 'overloadable' attribute}}
+void after_local_1(int) __attribute__((overloadable));
+void after_local_2(int);
 void after_local_3(int) __attribute__((overloadable));
 
 // Make sure we allow C-specific conversions in C.
@@ -106,8 +109,8 @@
   void foo(char *c) __attribute__((overloadable));
   void (*ptr1)(void *) = 
   void (*ptr2)(char *) = 
-  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
-  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
+  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}
 
   void (*specific1)(int *) = (void (*)(void *)) // expected-warning{{incompatible function pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
   void *specific2 = (void (*)(void *))
@@ -117,8 +120,8 @@
   

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D32332#751785, @george.burgess.iv wrote:

> I'd be happy with that approach. Do you like it, Aaron?


I think that makes a lot of sense.

> FWIW, I did a bit of archaeology, and it looks like the commit that added the 
> requirement that all overloads must have `overloadable` (r64414) did so to 
> keep users from "trying to be too sneaky for their own good." The issue it 
> was trying to solve was
> 
>   double sin(double) __attribute__((overloadable));
>   #include 
>   
>   // calls to `sin` are now mangled, which probably resulted in fun linker 
> errors.
> 
> 
> If we go back to no longer requiring some spelling of `overloadable` on all 
> (re)decls of this special non-mangled function, the above problem shouldn't 
> reappear.




https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

I'd be happy with that approach. Do you like it, Aaron?

FWIW, I did a bit of archaeology, and it looks like the commit that added the 
requirement that all overloads must have `overloadable` (r64414) did so to keep 
users from "trying to be too sneaky for their own good." The issue it was 
trying to solve was

  double sin(double) __attribute__((overloadable));
  #include 
  
  // calls to `sin` are now mangled, which probably resulted in fun linker 
errors.

If we go back to no longer requiring some spelling of `overloadable` on all 
(re)decls of this special non-mangled function, the above problem shouldn't 
reappear.


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

I'd like to suggest an alternative design: don't add a new attribute., and 
instead change the semantics of `__attribute__((overloadable))` to permit at 
most one non-overloadable function in an overload set. That one function would 
implicitly get the `transparently_overloadable` semantics: it can be overloaded 
by `overloadable` functions and doesn't get a mangled name. This is basically 
exactly how extern "C" and extern "C++" functions overload in C++: you can have 
up to one extern "C" function plus as many (overloadable) C++ functions as you 
like.

Does that seem reasonable? Or is there some reason we need or want an explicit 
attribute for this case?


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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();
+  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

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 98569.
george.burgess.iv marked 7 inline comments as done.
george.burgess.iv added a comment.

- Addressed all feedback
- Now we emit a warning when `transparently_overloadable` "overrides" 
`overloadable`, rather than an error


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,6 +3,9 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
+int var2 __attribute__((transparently_overloadable)); // expected-error{{'transparently_overloadable' attribute only applies to functions}}
+void params2(void) __attribute__((transparently_overloadable(12))); // expected-error {{'transparently_overloadable' attribute takes no arguments}}
+
 int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
 float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
@@ -106,8 +109,8 @@
   void foo(char *c) __attribute__((overloadable));
   void (*ptr1)(void *) = 
   void (*ptr2)(char *) = 
-  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
-  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
+  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}
 
   void (*specific1)(int *) = (void (*)(void *)) // expected-warning{{incompatible function pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
   void *specific2 = (void (*)(void *))
@@ -117,8 +120,8 @@
   void disabled(char *c) __attribute__((overloadable, enable_if(1, "The function name lies.")));
   // To be clear, these should all point to the last overload of 'disabled'
   void (*dptr1)(char *c) = 
-  void (*dptr2)(void *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
-  void (*dptr3)(int *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'int *' but has 'char *')}}
+  void (*dptr2)(void *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type ''}} expected-note@-5{{candidate function made ineligible by enable_if}} expected-note@-4{{candidate function made ineligible by enable_if}} expected-note@-3{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
+  void (*dptr3)(int *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expression of type ''}} expected-note@-6{{candidate function made ineligible by enable_if}} expected-note@-5{{candidate function made ineligible by enable_if}} expected-note@-4{{candidate function has type mismatch at 1st parameter (expected 'int *' but has 'char *')}}
 
   void *specific_disabled = 
 }
@@ -131,14 +134,14 @@
   void foo(char *c) __attribute__((overloadable));
   void foo(short *c) __attribute__((overloadable));
   foo(charbuf);
-  foo(ucharbuf); // expected-error{{call to 'foo' is ambiguous}} expected-note@131{{candidate function}} expected-note@132{{candidate function}}
-  foo(intbuf); // expected-error{{call to 'foo' is 

[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())
+  if (!A->isTransparent())

`const auto *`



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

`const auto *`



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

`const auto *`



Comment at: lib/Sema/SemaDecl.cpp:2921
+  const auto *NewOvl = New->getAttr();
+  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()) {
+  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(ND))
+if (auto *Ovl =
+FD->getMostRecentDecl()->getAttr())

`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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 97698.
george.burgess.iv added a comment.

We now require `transparently_overloadable` on all redeclarations. Allowing 
mixed `transparently_overloadable` and `overloadable` coming soon.


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,6 +3,9 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
+int var2 __attribute__((transparently_overloadable)); // expected-error{{'transparently_overloadable' attribute only applies to functions}}
+void params2(void) __attribute__((transparently_overloadable(12))); // expected-error {{'transparently_overloadable' attribute takes no arguments}}
+
 int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
 float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
@@ -106,8 +109,8 @@
   void foo(char *c) __attribute__((overloadable));
   void (*ptr1)(void *) = 
   void (*ptr2)(char *) = 
-  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
-  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
+  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}
 
   void (*specific1)(int *) = (void (*)(void *)) // expected-warning{{incompatible function pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
   void *specific2 = (void (*)(void *))
@@ -117,8 +120,8 @@
   void disabled(char *c) __attribute__((overloadable, enable_if(1, "The function name lies.")));
   // To be clear, these should all point to the last overload of 'disabled'
   void (*dptr1)(char *c) = 
-  void (*dptr2)(void *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
-  void (*dptr3)(int *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'int *' but has 'char *')}}
+  void (*dptr2)(void *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type ''}} expected-note@-5{{candidate function made ineligible by enable_if}} expected-note@-4{{candidate function made ineligible by enable_if}} expected-note@-3{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
+  void (*dptr3)(int *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expression of type ''}} expected-note@-6{{candidate function made ineligible by enable_if}} expected-note@-5{{candidate function made ineligible by enable_if}} expected-note@-4{{candidate function has type mismatch at 1st parameter (expected 'int *' but has 'char *')}}
 
   void *specific_disabled = 
 }
@@ -131,14 +134,14 @@
   void foo(char *c) __attribute__((overloadable));
   void foo(short *c) __attribute__((overloadable));
   foo(charbuf);
-  foo(ucharbuf); // expected-error{{call to 'foo' is ambiguous}} expected-note@131{{candidate function}} expected-note@132{{candidate function}}
-  foo(intbuf); // expected-error{{call to 'foo' is ambiguous}} expected-note@131{{candidate function}} expected-note@132{{candidate function}}

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> I'd like to understand this use case a little bit better. If you don't 
> perform the mangling, then choosing which overload gets called is already 
> based on the source order, isn't it? See https://godbolt.org/g/8b10Ns

I think I might have miscommunicated: `transparently_overloadable` only 
enforces no mangling on one function out of the set of functions with the same 
name. So,

  void foo(int) __attribute__((transparently_overloadable));
  void foo(float) __attribute__((overloadable));
  
  void bar() {
foo(1); // calls @foo
foo(1f); // calls @_Z3foof
  }



> The logic as to why overloadable is required on every redeclaration makes 
> sense, but also flies in the face of your rational for why you want a new 
> spelling of the same attribute that doesn't need to be on every 
> redeclaration. I worry about this difference being mildly confusing to user.

Yeah, good point. The more I think about that, the more I don't like the 
inconsistency; I'll drop that change.


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D32332#742796, @george.burgess.iv wrote:

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


Yeah, it would require the function name (then you could use the stringize 
preprocessor directive).

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

I'd like to understand this use case a little bit better. If you don't perform 
the mangling, then choosing which overload gets called is already based on the 
source order, isn't it? See https://godbolt.org/g/8b10Ns

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

The logic as to why overloadable is required on every redeclaration makes 
sense, but also flies in the face of your rational for why you want a new 
spelling of the same attribute that doesn't need to be on every redeclaration. 
I worry about this difference being mildly confusing to user.

> 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)`.

That's a fair reason for a new spelling rather than an argument.

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


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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)) {}

// 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

I'm adding Richard to the review because he may have opinions on this 
functionality.

Is there a reason we want the second spelling instead of making the current 
spelling behave in the manner you describe? While the change is fairly 
fundamental, I don't know that it would break code either (but I could be 
wrong). I think that forcing the user to choice which spelling of 
"overloadable" they want to use is not very user friendly.




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>;

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



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">;

Why should this be an error instead of simply an ignored attribute?



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

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.


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

ping :)


https://reviews.llvm.org/D32332



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.

One of the common use-cases for the `overloadable` attribute is to create small
wrapper functions around an existing function that was previously not
overloadable. For example:

  // C Library v1
  void foo(int i);
  
  
  // C Library v2
  // Note the __asm__("foo"); it ensures that foo isn't mangled. Our docs state
  // that the mangling isn't stable, so we encourage users to either use __asm__
  // to force a stable name, or to only use overloads with internal linkage
  __attribute__((overloadable))
  int foo(int i) __asm__("foo");
  
  __attribute__((overloadable))
  static inline int foo(long l) {
assert(l <= INT_MAX && l >= INT_MIN && "Out of bounds number!");
return foo(l);
  }

"Library v2" is suboptimal for a few reasons:

- It can break existing code; if users added `int foo(int);` declarations to 
their code, upgrading to library v2 requires that the users either remove these 
declarations or add `__attribute__((overloadable))` to them.
- The `__asm__` name is both redundant and easy to typo (or forget to update, 
or ...).
- The `__asm__` name also makes it quite a bit more difficult to hide all of 
this behind a macro, since we need a different name for each function.

This patch aims to fix these usability challenges with the idea of transparent
function overloads.

For any `overloadable` function in C, one of the overloads is allowed to be
transparent. Transparency means that the name will not be mangled, and later
redeclarations of that particular overload don't require
`__attribute__((overloadable))`.

Users can make an overload transparent by using the `transparently_overloadable`
spelling of `overloadable`.

This also includes a bugfix for how we handled `overloadable` functions declared
inside functions. I can factor that out if that would make life easier. :)


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/ItaniumMangle.cpp
  lib/AST/MicrosoftMangle.cpp
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,6 +3,9 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
+int var2 __attribute__((transparently_overloadable)); // expected-error{{'transparently_overloadable' attribute only applies to functions}}
+void params2(void) __attribute__((transparently_overloadable(12))); // expected-error {{'transparently_overloadable' attribute takes no arguments}}
+
 int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
 float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
@@ -106,8 +109,8 @@
   void foo(char *c) __attribute__((overloadable));
   void (*ptr1)(void *) = 
   void (*ptr2)(char *) = 
-  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
-  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) =  // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
+  void *vp_ambiguous =  // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}
 
   void (*specific1)(int *) = (void (*)(void *)) // expected-warning{{incompatible function pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
   void *specific2 = (void (*)(void *))
@@ -117,8 +120,8 @@
   void disabled(char *c) __attribute__((overloadable, enable_if(1, "The function name lies.")));
   // To be clear, these should all point to the last overload of 'disabled'
   void (*dptr1)(char *c) = 
-  void (*dptr2)(void *c) =  // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
-