Re: refactoring - share str2*int64 functions

Hi,

On 2019-07-17 07:55:39 +0000, Fabien COELHO wrote:
> > - 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?

> > 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?

> > 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.

> 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?

> 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? 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.

> 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, 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?

Greetings,

Andres Freund