[cfe-commits] [PATCH, RFC] Fix PR14881 (complex integer promotion rules)

[John McCall]

On Jan 25, 2013, at 12:08 PM, Bill Schmidt <wschmidt at linux.vnet.ibm.com> wrote:
> On Tue, 2013-01-22 at 15:50 -0800, John McCall wrote:
>> On Jan 16, 2013, at 1:41 PM, Bill Schmidt <wschmidt at linux.vnet.ibm.com> wrote:
>>> This patch addresses PR14881.
>>> 
>>> Prior to the patch, Clang does not properly promote types when a complex
>>> integer operand is combined with an integer via a binary operator, or when
>>> one is assigned to the other in either order.  This patch detects when
>>> promotion is needed (and permissible) and generates the necessary code.
>>> 
>>> I've included a rather extensive test case to exercise all the various
>>> cases, including signed and unsigned variants.  I believe that the necessary
>>> handling of signedness is already in place, and the generated code confirms
>>> this to my best understanding.  I hope that the test will run on all targets.
>>> I've assumed that no target has the same size operands for "char" and
>>> "long long," and that no target performs arithmetic on char operands without
>>> extending them to a larger format first.  If there are any targets for which
>>> this is not the case, the test case will need adjustment.  I'd appreciate
>>> any information about the validity of my assumptions before I turn the test
>>> loose on the world...
>>> 
>>> Thanks in advance for your review!
>> 
>> Minor point:
>> +    const QualType LHSBaseType = LHSComplexInt->getElementType();
>> We don't usually make local variables 'const' like this.  It looks like you
>> don't, either, except that you want to do it to QualType.  QualType is a very
>> small, cheap-to-copy value type — essentially a pointer — and you should
>> treat it like any other type with those properties.
>> 
>> Major point:  it doesn't look like you're considering signedness at all as
>> part of your promotions.  C wants us to do the usual arithmetic conversions
>> on the corresponding real type in order to select the common real type;
>> maybe you can extract those out from handleIntegerConversion somehow?
> 
> John, thanks again for the review!  I integrated the signedness rules
> from handleIntegerConversion as you suggested.  New version attached.
> Is this what you had in mind?


Something along these lines, but I'd really prefer if you could somehow
re-use the logic from handleIntegerConversion.  I think you could if you
just made that function take function pointers to do the casts on the left
and right parameters:

  typedef ExprResult PerformCastFn(Sema &S, Expr *operand, QualType toType);
  static QualType handleIntegerConversion(Sema &S, ExprResult &LHS,
                                        ExprResult &RHS, QualType LHSType,
                                        QualType RHSType, bool IsCompAssign,
                                        PerformCastFn *doLHSCast, PerformCastFn *doRHSCast)

The current use would just use a function like this:
  ExprResult doIntegralCast(Sema &S, Expr *op, QualType toType) {
    return S.ImpCastExprToType(op, toType, CK_IntegralCast);
  }

But whenever an operand is complex, you'd instead use:
  ExprResult doComplexIntegralCast(Sema &S, Expr *op, QualType toType) {
    return S.ImpCastExprToType(op, S.Context.getComplexType(toType), CK_ComplexIntegralCast);
  }

John.