[cfe-commits] [llvm-commits] [PATCH] Allow converting ConstantExprs to Instructions

[James Molloy]

Hi Renato,

OK, I'll add a comment detailing what we've discussed and pointing to that bug.

With those changes, is the patch OK to commit?

Cheers,

James
________________________________________
From: rengolin at gmail.com [rengolin at gmail.com] On Behalf Of Renato Golin [rengolin at systemcall.org]
Sent: 16 November 2012 21:09
To: James Molloy
Cc: llvm-commits at cs.uiuc.edu; cfe-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] [PATCH] Allow converting ConstantExprs to Instructions

On 16 November 2012 20:47, James Molloy <James.Molloy at arm.com> wrote:
> That seems like a heavyweight solution, and I also disagree with it because the current implementation as sent keeps all the implementation details of ConstantExpr (of which there are many) located and isolated in the same .cpp file. Adding implicit constructors which take a ConstantExpr would spread implementation details of ConstantExpr around the codebase.

I agree that that spreads ConstantExpr knowledge throughout the code,
but I don't agree there are many implementation details. Your
conversion function looks very small and straightforward.


> Also, Richard Smith mentioned at the dev conference that there may be a push to remove ConstantExpr completely, and if so my current implementation would be much easier to clean up :)

He's probably referring to this:
http://llvm.org/bugs/show_bug.cgi?id=10368

And this is a much better argument against constructors. I'm ok with
this being a separate function if the intent to remove ConstantExpr is
really going to happen soon-ish (next few releases), but this is not a
trivial change (I tried myself a few times), so I wouldn't hold my
breath.

Maybe you should add a comment to that effect.

Also, you seem to have good tests, so at least that's not going to
break silently.

--
cheers,
--renato

http://systemcall.org/