Hello Andres,
>> +"9223372036854775808" {
>> + /* yuk: special handling for minint */
>> + return MAXINT_PLUS_ONE_CONST;
>> + }
>
> Yikes, do we really need this? If we dealt with - in the lexer, we
> shouldn't need it, no?
I'm not sure how to handle unary minus constant in the lexer, given that
the same '-' character is also used as minus binary operator.
The proposed solution is functional and has a reduce overall impact (one
lexer and one parser rules added), so it looks good to me.
Probably other approaches are possible.
>> + /* this can't happen here, function called on int-looking strings. */
>
> This comment doesn't strike me as a good idea, it's almost guaranteed to
> be outdated at some point.
It is valid now, and it can be removed anyway.
> [...] Duplicating these routines is pretty ugly.
Sure.
> I really wish we had more infrastructure to avoid this (in particularly
> a portable way to report an error and jump out). But probably out of
> scope for this patches?
Yes.
Devising an appropriate client-side error handling/reporting
infrastructure is a non trivial tasks, and would cost more than a few
duplicate functions. "fprintf(stderr + return/exit" currently does the job
with minimal hassle. I do not think that this patch is the right one to
change how error are handle in postgres client applications.
>> + if (unlikely(end == str || *end != '\0'))
> Not sure I see much point in the unlikelys here, contrasting to the ones
> in the backend (and the copy for the frontend) it's extremely unlikley
> anything like this will ever be close to a bottleneck. In general, I'd
> strongly suggest not placing unlikelys in non-critical codepaths -
> they're way too often wrongly estimated.
I put "unlikely" where I really thought it made sense. I do not know when
they would have an actual performance impact, but I appreciate that they
convey information to the reader of the code, telling that this path is
expected not to be taken.
It can be removed anyway if you do not like it.
--
Fabien.
