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

[Richard Smith via cfe-commits]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200130/6cbb0a6e/attachment.html>