Re: [PATCHv2 2/2] [long] improvements to limit/ulimit API and doc (the rest)

["Daniel Shahaf" <d.s@xxxxxxxxxxxxxxxxxx>]

Stephane Chazelas wrote on Fri, 27 Nov 2020 20:13 +00:00:
> 2020-11-27 16:39:49 +0000, Daniel Shahaf:
> [...]
> > Please specify in the docstring the types, values, and meanings of the
> > formal parameters.
> 
> I've tried to address that in this v3 patch. Specifying the type
> (as in char*, int...) seems redundant though and not generally
> done in other functions, so I've left it out for now.

Of course, DRY; but still, notice how one of the ints is actually
a boolean, so the formal type information doesn't convey everything.

> +++ b/Src/Builtins/rlimits.c
> @@ -255,38 +267,214 @@ printrlim(rlim_t val, const char *unit)
> +/*
> + * Parse a string into the corresponding value based on the limit type
> + *
> + *   ZLIMTYPE_UNKNOWN / ZLIMTYPE_NUMBER:
> + *     decimal integer without sign only: raw value
> + *
> + *   ZLIMTYPE_TIME / ZLIMTYPE_MICROSECONDS:
> + *     <decimal> only gives seconds or microseconds depending on type
> + *     or:
> + *     [[hour:]min:]sec[.usec]
> + *     or:
> + *     <decimal> with h (hour), m (min), s (sec), ms, us suffix.
> + *
> + *   ZLIMTYPE_MEMORY:
> + *     <decimal> without suffix interpreted as KiB by "limit" (except for
> + *     RLIMIT_MSGQUEUE, see below) and based on resinfo.unit by "ulimit".
> + *
> + *     K/M/G/T/P/E suffix and same with iB suffix use 1024 factor
> + *     KB/MB/GB... use 1000 factor.
> + *
> + *     B for bytes (avoids that mess about default units).
> + *
> + * All suffixes are case insensitive.
> + *
> + * Arguments:
> + *   - s: string to parse
> + *   - lim: resource being limited (from which will derive the type and unit)
> + *   - ulimit: to specify whether we're being called by ulimit or not.
> + *             For ulimit, suffix-less limits are multiplied by the limit's
> + *             unit.

Suggest to add the word "boolean" to the description of this parameter.

Since you've already gone and described it in prose, I'd consider to
also rename it to something like
«multiply_suffixless_limits_by_implied_unit_p» (trailing _p for
"predicate", i.e., signifies the variable is a boolean), to avoid caller–callee coupling.

> + *   - err: (return value) error to be returned if any. If a non-NULL value is
> + *           stored there, zstrtorlimt failed and the return value is
> + *           irrelevant (though will be 0).

You don't actually say anywhere what the return value will be when there
_isn't_ an error.

Cheers,

Daniel

P.S.  As I said elsethread, feel free to stick this patch on a short-
      lived topic branch, if that's easier to you than posting revisions
      to a mailing list.

> + */
> +
>  /**/
>  static rlim_t
> -zstrtorlimt(const char *s, char **t, int base)
> +zstrtorlimt(const char *s, int lim, int ulimit, char **err)
>  {