rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
This looks good to me. I think we can further refine the handling of duplicate
diagnostics but I don't think that needs to be done as part of this bugfix.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:953
S.LookupQualifiedName(Members, RHSRec->getDecl());
- Members.suppressDiagnostics();
+ Members.suppressAccessDiagnostics();
for (NamedDecl *Op : Members)
----------------
Can we get duplicate diagnostics here if this lookup fails due to ambiguity? I
think we'll perform the same lookup again when looking for candidates if the
original operator was `!=`.
(I wonder if we can fix that by avoiding the duplicate lookup, rather than by
suppressing ambiguity diagnostics.)
================
Comment at: clang/lib/Sema/SemaOverload.cpp:964
S.getScopeForContext(EqFD->getEnclosingNamespaceContext()));
- NonMembers.suppressDiagnostics();
+ NonMembers.suppressAccessDiagnostics();
for (NamedDecl *Op : NonMembers) {
----------------
There can't be any access diagnostics here because this is a namespace-scope
lookup. You can just drop this call entirly.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:15083
case OR_Ambiguous:
- CandidateSet.NoteCandidates(
- PartialDiagnosticAt(Object.get()->getBeginLoc(),
- PDiag(diag::err_ovl_ambiguous_object_call)
- << Object.get()->getType()
- << Object.get()->getSourceRange()),
- *this, OCD_AmbiguousCandidates, Args);
+ if (!R.isAmbiguous())
+ CandidateSet.NoteCandidates(
----------------
I think we should also check whether the ambiguous viable functions were all
first declared in the same base class here. If so, then it still makes sense to
`NoteCandidates`, because the ambiguous lookup had nothing to do with the
ambiguous overload resolution.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:15293
case OR_Ambiguous:
- CandidateSet.NoteCandidates(
- PartialDiagnosticAt(OpLoc, PDiag(diag::err_ovl_ambiguous_oper_unary)
- << "->" << Base->getType()
- << Base->getSourceRange()),
- *this, OCD_AmbiguousCandidates, Base);
+ if (!R.isAmbiguous())
+ CandidateSet.NoteCandidates(
----------------
(Likewise here.)
================
Comment at: clang/lib/Sema/SemaOverload.cpp:15191
OverloadCandidateSet::iterator Best;
switch (CandidateSet.BestViableFunction(*this, OpLoc, Best)) {
case OR_Success:
----------------
shafik wrote:
> rsmith wrote:
> > shafik wrote:
> > > @rsmith if `R.isAmbiguous()` should we even check the overload
> > > candidates?
> > >
> > > Right now in `clang/test/SemaCXX/arrow-operator.cpp` we are getting both
> > > an ambiguous name lookup and overload diagnostic.
> > Hm, I think perhaps the ideal thing to do is: if `BestViableFunction`
> > returns `OR_Ambiguous`, the lookup was ambiguous, and we found viable
> > functions in multiple different base classes, then don't produce any
> > further diagnostics. That way, we still get good error recovery in the case
> > where the overloading is unambiguous despite the lookup being ambiguous, or
> > when the overloading is ambiguous for some reason unrelated to the
> > ambiguous lookup.
> >
> > (We'd want to do this in all the overloaded operator lookups in this file,
> > not just for `operator->`.)
> I only found one other location that matched this pattern if you meant a
> larger change let me know.
Ideally, we should make `AddMemberOperatorCandidates` (and
`LookupOverloadedBinOp`) inform its three callers whether its lookup was
ambiguous, so that they can also suppress the duplicate ambiguity diagnostics,
for unary operators, binary operators, and subscripting.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155387/new/
https://reviews.llvm.org/D155387
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
