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

[Renato Golin]

LGTM.

On 17 November 2012 12:34, James Molloy <James.Molloy at arm.com> wrote:
> 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/
>
>



-- 
cheers,
--renato

http://systemcall.org/