Re: refactoring - share str2*int64 functions - Mailing list pgsql-hackers

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