[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<OverloadableAttr>();
+                })) &&
+           "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 *) = &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}}
----------------
rsmith wrote:
> It'd be nice to check in these changes from @NNN -> @-M separately.
r305947.


https://reviews.llvm.org/D32332