>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Tom> * I dislike using castNode() in places where the code has just
Tom> explicitly verified the node type, which is true in both places
Tom> where you used it here. The assertion is just bulking the code up
Tom> to no purpose, and it creates an unnecessary discrepancy between
Tom> older and newer code.
hmm... fair point, I'll think about it
Tom> * As you have it here, a construct such as
Tom> ConvertRowtypeExpr(ConvertRowtypeExpr(ConvertRowtypeExpr(Const)))
Tom> will laboriously perform each of the intermediate steps, which
Tom> seems like exactly the case we're trying to prevent at runtime. I
Tom> wonder whether it is worth stripping off ConvertRowtypeExpr's
Tom> before the recursive eval_const_expressions_mutator call to
Tom> prevent that. You'd still want the check after the call, to handle
Tom> situations where something more complex got simplified to a
Tom> ConvertRowtypeExpr; and this would also complicate getting the
Tom> convertformat right. So perhaps it's not worth the trouble, but I
Tom> thought I'd mention it.
I think it's not worth the trouble.
Tom> * I find the hardwired logic about how to merge distinct
Tom> convertformat values a bit troublesome. Maybe use Min() instead?
Tom> Although there is not currently any expectation about the ordering
Tom> of that enum ...
I considered using Min() but decided against it specifically _because_
there was no suggestion either in the enum definition, or in any other
use of CoercionForm anywhere, that the order of values was intended to
be significant. Since there's only one value for "implicit", it seemed
better to work from that.
--
Andrew (irc:RhodiumToad)
