LGTM. On 17 November 2012 12:34, James Molloy <[email protected]> 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: [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/ > >
-- cheers, --renato http://systemcall.org/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
