>>>>> "Tom" == Tom Lane <t...@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)
