Parenthesize MIN/MAX macro arguments - #9928
Conversation
|
See #9807 (comment) and #9807 (comment), related. |
fallenmi
left a comment
There was a problem hiding this comment.
The defensive parentheses fix both precedence classes I could reproduce. Against the production ImagingUtils.h, exact PR base cacb5139bf9abe70afe2cd533b319d5c66a48867 computes MAX(1, 10 | 4) as 1 and MIN(5, 1 ? 2 : 9) as 5; Clang with -Werror also rejects the first expansion. Exact head a34b2219fd573b69517d9468e04b8ee2992b80cb produces the expected 14 and 2 and compiles cleanly with -std=c11 -Wall -Wextra -Werror.
I also merged that exact head cleanly into current protected main e41083f383c9cd3db95de52564cc0b6452313d4a and repeated the oracle. The focused suites covering all current macro-consumer groups (Convert, BoxBlur, QuantOctree, and TiffDecode) passed on both exact head and the current-main merge: 476 passed, 4 unrelated platform/optional-data skips in each tree. git diff --check is clean. At final recheck, all 55 exact-head check runs and all 11 check suites were successful, as were both legacy statuses.
Reviewed with OpenAI Codex assistance; exact current-head and current-main-merge behavior was reproduced locally. I found no blocker in the measured scope.
#9921 replaced duplicated variants of
MIN/MAXmacros with the variant that has certain subtle precedence issues, see e.g. the trivial exampleor
MIN(a, flag ? x : y)which would expand to(a < flag ? x : y ? a : flag ? x : y), which parses as(a < flag) ? x : (y ? a : (flag ? x : y)), which is probably not what the programmer expects.While right now it looks (quick visual grep) like there were no call sites affected, I think it's a good idea to have the predictable variant of these macros, instead of the subtly wrong one.