On Mon, Jul 26, 2021 at 9:37 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> + <formalpara>
> + <title>non-negative</title>
> + <para>
> + Do not use <quote>non-negative</quote> word in error messages as it looks
> + ambiguous. Instead, use <quote>foo must be an integer value greater than
> + zero</quote> or <quote>foo must be an integer value greater than or equal
> + to zero</quote> if option <quote>foo</quote> expects an integer value.
> + </para>
> + </formalpara>
>
> This description looks a bit redundant. And IMO it's better to also document
> how "non-negative" is ambiguous. So what about the following description,
> instead? I replaced this description with the following. Patch attached. I
> also uppercased the first character "n" of "non-negative" at the title for
> the sake of consistency with other items.
>
> + <formalpara>
> + <title>Non-negative</title>
> + <para>
> + Avoid <quote>non-negative</quote> as it is ambiguous
> + about whether it accepts zero. It's better to use
> + <quote>greater than zero</quote> or
> + <quote>greater than or equal to zero</quote>.
> + </para>
> + </formalpara>
LGTM.
> - /* Number of workers should be non-negative. */
> + /* Number of parallel workers should be greater than zero. */
> Assert(nworkers >= 0);
>
> This should be "greater than or equal to zero", instead? Anyway since this is
> comment not an error message, and also there are still other comments using
> "non-negative", I don't think we need to change only this comment for now. So
> I excluded this change from the patch. Maybe we can get rid of all
> "non-negative" from comments and documents later *if* necessary.
+1 to not change any code comments.
> - errmsg("repeat count size must be a
> non-negative integer")));
> + errmsg("repeat count size must be greater
> than or equal to zero")));
>
> - errmsg("number of workers must be a
> non-negative integer")));
> + errmsg("number of workers must be greater
> than or equal to zero")));
>
> Isn't it better to replace "be greater" with "be an integer value greater"? I
> applied this to the patch.
+1.
Thanks for the v8 patch, it LGTM.
Regards,
Bharath Rupireddy.
