Re: [PATCH 01/14] numparse: new module for parsing integral numbers

On Tuesday, March 17, 2015, Michael Haggerty [off-list ref] wrote:

Perhaps use a /* one-line style comment */ to reduce vertical space
consumption a bit, thus make it (very slightly) easier to run the eye
over the code.

+       errno = 0;
+       l = strtol(s, (char **)&end, flags & NUM_BASE_MASK);
+       if (errno) {
+               if (errno == ERANGE) {
+                       if (!(flags & NUM_SATURATE))
+                               return -NUM_SATURATE;
+               } else {
+                       return -NUM_OTHER_ERROR;
+               }
+       }

Would it reduce cognitive load slightly (and reduce vertical space
consumption) to rephrase the conditionals as:

    if (errno == ERANGE && !(flags & NUM_SATURATE))
        return -NUM_SATURATE;

    if (errno && errno != ERANGE)
        return -NUM_OTHER_ERROR;

or something similar?

More below.

Did you mean s/unsigned int/signed int/ ?

+ *
+ *     if (convert_i(s, NUM_SLOPPY, &result))
+ *             die("...");
+ */
+
+/*
+ * The lowest 6 bits of flags hold the numerical base that should be
+ * used to parse the number, 2 <= base <= 36. If base is set to 0,
+ * then NUM_BASE_SPECIFIER must be set too; in this case, the base is
+ * detected automatically from the string's prefix.

Does this restriction go against the goal of making these functions
convenient, even while remaining strict? Is there a strong reason for
not merely inferring NUM_BASE_SPECIFIER when base is 0? Doing so would
make it consistent with strto*l() without (I think) introducing any
ambiguity.

+ */
+/*
+ * Number parsing functions:
+ *
+ * The following functions parse a number (long, unsigned long, int,
+ * or unsigned int respectively) from the front of s, storing the
+ * value to *result and storing a pointer to the first character after
+ * the number to *endptr. flags specifies how the number should be
+ * parsed, including which base should be used. flags is a combination
+ * of the numerical base (2-36) and the NUM_* constants above (see).

"(see)" what?

+ * Return 0 on success or a negative value if there was an error. On
+ * failure, *result and *entptr are left unchanged.
+ *
+ * Please note that if NUM_TRAILING is not set, then it is
+ * nevertheless an error if there are any characters between the end
+ * of the number and the end of the string.

Again, on the subject of convenience, why this restriction? The stated
purpose of the parse_*() functions is to parse the number from the
front of the string and return a pointer to the first non-numeric
character following. As  a reader of this API, I interpret that as
meaning that NUM_TRAILING is implied. Is there a strong reason for not
inferring NUM_TRAILING for the parse_*() functions at the API level?
(I realize that the convert_*() functions are built atop parse_*(),
but that's an implementation detail.)

+ */
+
+int parse_l(const char *s, unsigned int flags,
+           long *result, char **endptr);

Do we want to perpetuate the ugly (char **) convention for 'endptr'
from strto*l()? Considering that the incoming string is const, it
seems undesirable to return a non-const pointer to some place inside
that string.

+/*
+ * Number conversion functions:
+ *
+ * The following functions parse a string into a number. They are
+ * identical to the parse_*() functions above, except that the endptr
+ * is not returned. These are most useful when parsing a whole string
+ * into a number; i.e., when (flags & NUM_TRAILING) is unset.

I can formulate arguments for allowing or disallowing NUM_TRAILING
with convert_*(), however, given their purpose of parsing the entire
string into a number, as a reader of the API, I would kind of expect
the convert_*() functions to ensure that NUM_TRAILING is not set
(either by forcibly clearing it or erroring out as an inconsistent
request if it is set).

+ */
+static inline int convert_l(const char *s, unsigned int flags,
+                           long *result)
+{
+       return parse_l(s, flags, result, NULL);
+}