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: [email protected] [[email protected]] On Behalf Of Renato Golin [[email protected]] Sent: 16 November 2012 21:09 To: James Molloy Cc: [email protected]; [email protected] Subject: Re: [llvm-commits] [PATCH] Allow converting ConstantExprs to Instructions On 16 November 2012 20:47, James Molloy <[email protected]> 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/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
