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
