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

[Bill Schmidt]

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.

Hm, yes, no idea why that crept in...

> 
> 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?

Thanks, I'll have a look.  I appreciate the review!

Bill

> 
> John.