[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<OverloadableAttr>();
+                })) &&
+           "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 *) = &foo;
-  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type '<overloaded function type>'}} expected-note at 105{{candidate function}} expected-note at 106{{candidate function}}
-  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an expression of incompatible type '<overloaded function type>'}} expected-note at 105{{candidate function}} expected-note at 106{{candidate function}}
+  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type '<overloaded function type>'}} expected-note at -4{{candidate function}} expected-note at -3{{candidate function}}
+  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an expression of incompatible type '<overloaded function type>'}} expected-note at -5{{candidate function}} expected-note at -4{{candidate function}}
----------------
It'd be nice to check in these changes from @NNN -> @-M separately.


https://reviews.llvm.org/D32332