On 28.11.2025 22:28, Tom Lane wrote: > David Geier <[email protected]> writes: >> On 27.11.2025 00:03, Chao Li wrote: >>> This is a large patch, I just take a quick look, and found that: >>> - *phoned_word = palloc(sizeof(char) * strlen(word) + 1); >>> + *phoned_word = palloc_array(char, strlen(word) + 1); >>> And >>> - params = (const char **) palloc(sizeof(char *)); >>> + params = palloc_object(const char *); >>> Applying palloc_array and palloc_object to char type doesn’t seem to >>> improve anything. > >> You mean because sizeof(char) is always 1 and hence we could instead >> simply write: >> *phoned_word = palloc(strlen(word) + 1); >> params = palloc(1); >> I think the _array and _object variants are more expressive and for sure >> don't make the code less readable. > > Yeah, I agree these particular changes seem fine. When you're doing > address arithmetic for a memcpy or such, it may be fine to wire in an > assumption that sizeof(char) == 1, but I think doing that in other > contexts is not particularly good style. > > Another thing to note is that the proposed patch effectively changes > the expression evaluation order: > > - *phoned_word = palloc((sizeof(char) * strlen(word)) + 1); > + *phoned_word = palloc(sizeof(char) * (strlen(word) + 1)); > > Now, there's not actually any difference because sizeof(char) is 1, > but if it hypothetically weren't, the new version is likely more > correct. Presumably the +1 is meant to allow room for a trailing \0, > which is a char.
Oh right. Good catch! > It'd be a good idea to review the patch to see if there are any > places where semantics are changed in a less benign fashion... I intentionally tried to avoid any semantic changes but it's of course possible something slipped through by accident. It's some ~1700 occurrences in the patch. If it takes ~10 seconds to check a single, it would take ~4.7h to review the entire patch. If no one is willing to take this on, I could split up the patch in 4 to 5 that each can be reviewed by someone else. -- David Geier
