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@105{{candidate function}} expected-note@106{{candidate
function}}
- void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an
expression of incompatible type '<overloaded function type>'}}
expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+ void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void
(*)(int *)' with an expression of incompatible type '<overloaded function
type>'}} expected-note@-4{{candidate function}} expected-note@-3{{candidate
function}}
+ void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an
expression of incompatible type '<overloaded function 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
