[cfe-commits] [PATCH][Review request] - Merge TypesCompatibleExprClass into BinaryTypeTraitExpr

[Douglas Gregor]

On Dec 8, 2010, at 3:10 AM, Francois Pichet wrote:

> This patch remove the TypesCompatibleExprClass AST node and merge it
> into BinaryTypeTraitExpr.
> A lot of - sign there.
> 
> The things to watch out are :
> 
>  return Owned(new (Context) BinaryTypeTraitExpr(KWLoc, BTT, LhsTSInfo,
>                                                  RhsTSInfo, Value, RParen,
> -                                                 Context.BoolTy));
> +                                                 (BTT == BTT_TypeCompatible) ?
> +                                                 Context.IntTy :
> Context.BoolTy));
> 
> BTT_TypeCompatible must have an int type otherwise some lit tests fail.

It's fine for different type traits to have different return types, but please use a switch() that lists all of the cases. That when, when a new type trait gets added, we'll be forced to update the switch appropriately.

FWIW, BTT_TypeCompatible has 'int' type because it's a GNU C extension.

> 
> Also this:
>   Value *VisitBinaryTypeTraitExpr(const BinaryTypeTraitExpr *E) {
> -    return llvm::ConstantInt::get(Builder.getInt1Ty(), E->getValue());
> +    return llvm::ConstantInt::get(ConvertType(E->getType()), E->getValue());
>   }

Yes, that makes sense.

Everything else looks good, thanks!

	- Doug