On 20/11/2025 05:36, Chao Li wrote:
I have no concern if we decide to no longer support tailing spaces, while I
still got a couple of small comments:
1
```
-/* return whether str matches "^\s*[-+]?[0-9]+$" */
+/*
+ * Return whether str matches "^\s*[-+]?[0-9]+$"
+ *
+ * This should agree with strtoint64() on what's accepted, ignoring overflows.
+ */
static bool
is_an_int(const char *str)
```
Here you added a comment saying "ignoring overflows”, yes, is_an_int() doesn’t
check if the integer overflows.
But looking at where the function is called:
```
else if (is_an_int(var->svalue))
{
/* if it looks like an int, it must be an int without overflow
*/
int64 iv;
if (!strtoint64(var->svalue, false, &iv))
return false;
setIntValue(&var->value, iv);
}
```
The comment says “it must be an int without overflow”, so this comment should
be updated as well.
Hmm, I don't think it's wrong as it is, and this patch doesn't change
that behavior. That comment is a little vague though.
How about the following phrasing:
/*
* If it looks like an integer, treat it as such. If it turns out to be
* too large for 'int64', return failure rather than fall back to 'double'.
*/
I don't feel the urge to refactor this myself right now, but we probably
could simplify this further. For example, I wonder if we should remove
is_an_int() altogether and rely on strtoi64() to return failure if the
input does't look like a integer. Also, strtodouble() is never called
with "errorOk != false".
2
```
+ if (unlikely(errno != 0))
{
- int8 digit = (*ptr++ - '0');
-
- if (unlikely(pg_mul_s64_overflow(tmp, 10, &tmp)) ||
- unlikely(pg_sub_s64_overflow(tmp, digit, &tmp)))
- goto out_of_range;
+ if (!errorOK)
+ pg_log_error("value \"%s\" is out of range for type
bigint", str);
+ return false;
}
```
Here we log an “out out range” error when errno is not 0, which is inaccurate,
we should check ERANGE.
strtoi64() maps to strtol()/strtoll(), the functions could return more errors
than ERANGE.
Good point. The existing strtodouble() function, which uses strtod(),
has the same issue (per POSIX spec at
https://pubs.opengroup.org/onlinepubs/000095399/functions/strtod.html):
These functions may fail if:
[EINVAL]
[CX] [Option Start] No conversion could be performed. [Option End]
Fixed that and committed. Thanks for the review!
- Heikki
