- The str->integer conversion routines, which actually have very
similar characteristics to the strtol families as they remove trailing
whitespaces first, check for a sign, etc, except that they work only
on base 10. And here we get into a state where pg_scanint8 should be
actually called pg_strtoint64,
I just removed that:-)
What do you mean by that?
That I renamed something from a previous patch version and someone is
complaining that I did.
with an interface inconsistent with its int32/int16 relatives now only
in the backend.
We can, but I'm not at ease with changing the error handling approach.
Why?
If a function reports an error to log, it should keep on doing it,
otherwise there would be a regression.
Could we consider more consolidation here please? Like moving the whole
set to src/common/?
My initial plan was simply to remove direct code duplications between
front-end and back-end, not to re-engineer the full set of string to int
conversion functions:-)
Well, if you expose functions to more places - e.g. now the whole
frontend - it becomes more important that they're reasonably designed.
I can somehow only agree with that. Note that the contraposite assumption
that badly designed functions would be okay for backend seems doubtful.
On the re-engineering front: Given the various points on the thread,
ISTM that there should probably be two behaviors for str to
signed/unsigned int{16,32,64}, and having only one kind of signature
for all types would be definitely better.
I don't understand why we'd want to have different behaviours for
signed/unsigned? Maybe I'm mis-understanding your sentence, and you just
mean that there should be one that throws, and one that returns an
errorcode?
Yep for the backend (if reporting an error generates a longjump), for the
frontend there is no exception mechanism, so it is showing the error or
not to stderr, and returning whether it was ok.
Another higher-level one which possibly adds an error message (stderr for
front-end, log for back-end).
Is there actually any need for a non-backend one that has an
error-message?
Pgbench uses it. If the function is shared and one is loging something, it
looks ok to send to stderr for front-end?
I'm not convinced that in the frontend it's very useful to have such a
function that exits - in the backend we have a much more complete way to
handle that, including pointing to the right location in the query
strings etc.
Sure. There is not exit though, just messages to stderr and return false.
One choice is whether there are two functions (the higher one calling the
lower one and adding the messages) or just one with a boolean to trigger the
messages. I do not have a strong opinion. Maybe one function would be
simpler. Andres boolean-compatible enum return looks like a good option.
The boolean makes the calling code harder to understand, the function
slower,
Hmmm. So I understand that you would prefer 2 functions, one raw (fast)
one and the other with the other with the better error reporting facity,
and the user must chose the one they like. I'm fine with that as well.
and the code harder to grep.
Overall, this leads to something like:
enum { STRTOINT_OK, STRTOINT_OVERFLOW_ERROR, STRTOINT_SYNTAX_ERROR }
pg_strto{,u}int{8?,16,32,64}
(const char * string, const TYPE * result, const bool verbose);
What's with hthe const for the result? I assume that was just a copy&pasto?
Yep. The pointer is constant, not the value pointed, maybe it should be
"TYPE * const result" or something like that. Or no const at all on
result.
--
Fabien.
