gregfelice commented on PR #2407:
URL: https://github.com/apache/age/pull/2407#issuecomment-4972869139
@sfc-gh-dachristensen — I think this change is a no-op, and the title
overstates it. Recommending we close unless I'm missing something.
The diff is:
```c
- if (strcmp(str, ""))
+ if (strcmp(str, "") != 0)
```
These are identical in C. `strcmp()` returns `0` when the strings are equal
and non-zero otherwise, and `if (x)` already tests `x != 0`. So both forms mean
"if `str` is **not** empty" and compile to the same thing. There is no inverted
logic here — the existing code is already correct.
The semantics of the surrounding block are also right as-is: `strtol()` sets
`str` to the first unparsed character, so `strcmp(str, "")` being true means
there was trailing garbage after the number (e.g. `"1abc"`), and the code
correctly bails with `PG_RETURN_NULL()`. An empty leftover means the whole key
parsed cleanly as an integer.
If the actual concern is the one Copilot describes — that an **empty key**
`""` is accepted and silently treated as index `0`, because `strtol("")`
returns 0 with `str` pointing at the terminator so the guard passes — then that
is a real issue, but this diff does not fix it. That would need an explicit
check that the key is non-empty (and ideally an `errno`/`ERANGE` check around
`strtol`) before using `lindex`.
Happy to look at a revised PR that addresses the empty-key case with a
regress test. As it stands I'd close this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
