On Tue, Jun 11, 2024 at 09:31:39AM -0400, Joseph Koshakow wrote: > tmp is an uint16 here, it seems like you might have read it as an > int16? We would need some helper method like > > static inline bool > pg_neg_u16_overflow(uint16 a, int16 *result); > > and then we could replace that whole chunk with something like > > if (unlikely(pg_neg_u16_overflow(tmp, &result))) > goto out_of_range; > else > return result; > > > that pattern shows up a lot in this file, but I was worried that it > wasn't useful as a general purpose function. Happy to add it > though if you still feel otherwise.
I personally find that much easier to read. Since the existing open-coded overflow check is apparently insufficient, I think there's a reasonably strong case for centralizing this sort of thing so that we don't continue to make the same mistakes. >> Ah, I see. Joe's patch does that in one place. It's probably worth doing >> that in the other places this "just-barefly overflow" comment appears >> IMHO. > > The only other place I found this comment was in > `make_timestamp_internal`. I've updated that function and added some > tests. I also manually verified that the behavior matches before and > after this patch. tm2timestamp() in src/interfaces/ecpg/pgtypeslib/timestamp.c has the same comment. The code there looks very similar to the code for tm2timestamp() in the other timestamp.c... -- nathan
