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

On Tue, Jul 16, 2019 at 11:16:31AM +1200, Thomas Munro wrote:
> Cool.  I'm not exactly sure when we should include 'pg_' in identifier
> names.  It seems to be used for functions/macros that wrap or replace
> something else with a similar name, like pg_pwrite(),
> pg_attribute_noreturn(), ...  In this case it's just our own code that
> we're moving, so I'm wondering if we should just call it scanint8().

FWIW, I was looking forward to putting my hands on this patch and try
to get it merged so as we can get rid of those duplications.  Here are
some comments.

+#ifdef FRONTEND
+       fprintf(stderr,
+               "invalid input syntax for type %s: \"%s\"\n",
"bigint", str);
+#else
+       ereport(ERROR,
+               (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+                errmsg("invalid input syntax for type %s: \"%s\"",
+                       "bigint", str)));
+#endif
Have you looked at using the wrapper pg_log_error() here?

+extern bool pg_scanint8(const char *str, bool errorOK, int64
*result);
+extern uint64 pg_strtouint64(const char *str, char **endptr, int
base);
Hmm.  With this patch we have strtoint and pg_strtouint64, which makes
the whole set inconsistent.

+
 #endif                         /* COMMON_STRING_H */
Noise diff..

numutils.c also has pg_strtoint16 and pg_strtoint32, so the locations
become rather inconsistent with inconsistent APIs for the manipulation
of int2 and int4 fields, and scanint8 is just a derivative of the same
logic.  We have two categories of routines here:
- The wrappers on top of strtol and strtoul & co, which are named
respectively strtoint and pg_strtouint64 with the patch.  The naming
part is inconsistent, and we only handle uint64 and int32.  We don't
need to worry about int64 and uint32 because they are not used?
That's fine by me, but at least let's have a consistent naming.
Prefixing the functions with pg_* is a better practice in my opinion
as we will unlikely run into conflicts this way.
- 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, with an interface inconsistent with its
int32/int16 relatives now only in the backend.  Could we consider more
consolidation here please?  Like moving the whole set to src/common/?

> If you agree, I think this is ready to commit.

Thomas, are you planning to look at this patch as committer?  I had it
in my agenda, and was planning to look at it sooner than later.  Now
if you are on it, I won't step on your toes.
--
Michael