[clang] 1f3f8c3 - PR44721: Don't consider overloaded operators for built-in comparisons

[Hans Wennborg via cfe-commits]

Yes, go ahead.

On Fri, Jan 31, 2020 at 2:19 AM Richard Smith <richard at metafoo.co.uk> wrote:
>
> Hi Hans,
>
> This is a pretty safe bugfix for a new feature in Clang 10. OK for branch?
>
> On Thu, 30 Jan 2020 at 17:17, Richard Smith via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>>
>>
>> Author: Richard Smith
>> Date: 2020-01-30T17:16:50-08:00
>> New Revision: 1f3f8c369a5067a132c871f33a955a7feaea8534
>>
>> URL: https://github.com/llvm/llvm-project/commit/1f3f8c369a5067a132c871f33a955a7feaea8534
>> DIFF: https://github.com/llvm/llvm-project/commit/1f3f8c369a5067a132c871f33a955a7feaea8534.diff
>>
>> LOG: PR44721: Don't consider overloaded operators for built-in comparisons
>> when building a defaulted comparison.
>>
>> As a convenient way of asking whether `x @ y` is valid and building it,
>> we previouly always performed overload resolution and built an
>> overloaded expression, which would both end up picking a builtin
>> operator candidate when given a non-overloadable type. But that's not
>> quite right, because it can result in our finding a user-declared
>> operator overload, which we should never do when applying operators
>> non-overloadable types.
>>
>> Handle this more correctly: skip overload resolution when building
>> `x @ y` if the operands are not overloadable. But still perform overload
>> resolution (considering only builtin candidates) when checking validity,
>> as we don't have any other good way to ask whether a binary operator
>> expression would be valid.
>>
>> Added:
>>
>>
>> Modified:
>>     clang/lib/Sema/SemaDeclCXX.cpp
>>     clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
>>
>> Removed:
>>
>>
>>
>> ################################################################################
>> diff  --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
>> index 814a3c64eeba..65526e4020cf 100644
>> --- a/clang/lib/Sema/SemaDeclCXX.cpp
>> +++ b/clang/lib/Sema/SemaDeclCXX.cpp
>> @@ -7373,7 +7373,14 @@ class DefaultedComparisonAnalyzer
>>      ///   resolution [...]
>>      CandidateSet.exclude(FD);
>>
>> -    S.LookupOverloadedBinOp(CandidateSet, OO, Fns, Args);
>> +    if (Args[0]->getType()->isOverloadableType())
>> +      S.LookupOverloadedBinOp(CandidateSet, OO, Fns, Args);
>> +    else {
>> +      // FIXME: We determine whether this is a valid expression by checking to
>> +      // see if there's a viable builtin operator candidate for it. That isn't
>> +      // really what the rules ask us to do, but should give the right results.
>> +      S.AddBuiltinOperatorCandidates(OO, FD->getLocation(), Args, CandidateSet);
>> +    }
>>
>>      Result R;
>>
>> @@ -7826,10 +7833,14 @@ class DefaultedComparisonSynthesizer
>>        return StmtError();
>>
>>      OverloadedOperatorKind OO = FD->getOverloadedOperator();
>> -    ExprResult Op = S.CreateOverloadedBinOp(
>> -        Loc, BinaryOperator::getOverloadedOpcode(OO), Fns,
>> -        Obj.first.get(), Obj.second.get(), /*PerformADL=*/true,
>> -        /*AllowRewrittenCandidates=*/true, FD);
>> +    BinaryOperatorKind Opc = BinaryOperator::getOverloadedOpcode(OO);
>> +    ExprResult Op;
>> +    if (Type->isOverloadableType())
>> +      Op = S.CreateOverloadedBinOp(Loc, Opc, Fns, Obj.first.get(),
>> +                                   Obj.second.get(), /*PerformADL=*/true,
>> +                                   /*AllowRewrittenCandidates=*/true, FD);
>> +    else
>> +      Op = S.CreateBuiltinBinOp(Loc, Opc, Obj.first.get(), Obj.second.get());
>>      if (Op.isInvalid())
>>        return StmtError();
>>
>> @@ -7869,8 +7880,12 @@ class DefaultedComparisonSynthesizer
>>        llvm::APInt ZeroVal(S.Context.getIntWidth(S.Context.IntTy), 0);
>>        Expr *Zero =
>>            IntegerLiteral::Create(S.Context, ZeroVal, S.Context.IntTy, Loc);
>> -      ExprResult Comp = S.CreateOverloadedBinOp(Loc, BO_NE, Fns, VDRef.get(),
>> -                                                Zero, true, true, FD);
>> +      ExprResult Comp;
>> +      if (VDRef.get()->getType()->isOverloadableType())
>> +        Comp = S.CreateOverloadedBinOp(Loc, BO_NE, Fns, VDRef.get(), Zero, true,
>> +                                       true, FD);
>> +      else
>> +        Comp = S.CreateBuiltinBinOp(Loc, BO_NE, VDRef.get(), Zero);
>>        if (Comp.isInvalid())
>>          return StmtError();
>>        Sema::ConditionResult Cond = S.ActOnCondition(
>>
>> diff  --git a/clang/test/CXX/class/class.compare/class.compare.default/p3.cpp b/clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
>> index 3d0ab2c5bde6..81a48a393a06 100644
>> --- a/clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
>> +++ b/clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
>> @@ -190,3 +190,15 @@ bool operator<(const G&, const G&);
>>  bool operator<=(const G&, const G&);
>>  bool operator>(const G&, const G&);
>>  bool operator>=(const G&, const G&);
>> +
>> +namespace PR44721 {
>> +  template <typename T> bool operator==(T const &, T const &) { return true; }
>> +  template <typename T, typename U> bool operator!=(T const &, U const &) { return true; }
>> +  template <typename T> int operator<=>(T const &, T const &) { return 0; }
>> +
>> +  struct S {
>> +    friend bool operator==(const S &, const S &) = default;
>> +    friend bool operator<=>(const S &, const S &) = default;
>> +    int x;
>> +  };
>> +}
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits