[PATCH] D32332: Add support for transparent overloadable functions in clang
George Burgess IV via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 21 13:15:11 PDT 2017
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
More information about the cfe-commits
mailing list