On Thu, Oct 21, 2021 at 4:47 PM Giovanni Mascellani gmascellani@codeweavers.com wrote:
Hi,
Il 21/10/21 16:17, Matteo Bruni ha scritto:
A detail that's not obvious from the patch is that expr_compatible_data_types() already expects (and accepts) the case where a vector and a matrix with the same component count are used together, so this in practice fixes expr_common_shape() to conform to that.
Just FTR, in general I like to keep as close as possible the point where a certain precondition is checked and the point where the same condition is used, as I find that code is more readable that way. It could be argued that the mismatch between what expr_compatible_data_types() checks and the assumption in expr_common_shape() might have been due to the fact that there was too much between where the condition was checked and where it was used.
At some point I had a patch to fix expr_common_shape() in order to conform to this principle, but it was broken for other reasons, and eventually it bitrotted and I dropped it. But if you think that change is valuable, I can rewrite it.
Yeah, in general I agree with the principle. WRT this specific case, it's probably not a big deal. You're going to touch related code, if the fixup patch comes out naturally I'm happy to review it. Otherwise, probably it's not worth the effort to go out of your way to write it.