Re: coding standards

[don fong]

Clark,

Just took a look at the code and it is an int:


declaring boolean quantities as int is a common practice in old C code.
indeed, all the boolean vars in this program seem to be declared as int.
at least, i don't see anything declared as bool.

declared type notwithstanding, in the context of subst.c, check_nullness
is being used as a boolean.  unfortunately, in retrospect the snippets i
posted are not as clear as i had thought.  (is there a way to provide a URL
for subst.c that links back to the devel branch at git.savannah.gnu.org?
as can be done with github or gitlab.)

consider:

* there is only one line where check_nullness is set to a non-zero value.
* that line is executed at most once.
* it does check_nullness++ which is another common practice for setting
  a boolean variable in C code.

 8449       check_nullness++;

* there are at least 2 other references to check_nullness in a boolean
context:

 8689   if (check_nullness)

 8687   var_is_null = check_nullness && (var_is_set == 0 || *temp == 0);

while these boolean-style references may not prove that check_nullness
is boolean, it at least shows that using it as a boolean is not inconsistent
with existing practices.

if you agree it's a boolean, wouldn't it be better to test its truth/falsity
directly instead of comparing it to false (0)?  the latter seems like a
typical "trap" that coding pundits warn against.

consistency would also argue for using the same variable name,
check_nullness
(as in my patch) instead of check_null (as in the altered version).










On Sun, Mar 4, 2018 at 6:15 PM, Clark Wang <dearvoid@gmail.com> wrote:

> On Mon, Mar 5, 2018 at 9:13 AM, don fong <dfong@dfong.com> wrote:
>
>> Clark, thanks for your answer.
>>
>> I use ``if (flag)'' only when `flag' is a boolean.
>>
>>
>> but in this case, it *is* a boolean, as i stated, and as can be seen in
>> subst.c:
>>
>> +    {
>> +      if (check_nullness)
>> +          report_error (_("%s: parameter null or not set"), name);
>> +      else
>> +          report_error (_("%s: parameter is not set"), name);
>> +    }
>>
>
> Just took a look at the code and it is an int:
>
> @@ -6849,8 +6849,9 @@ parameter_brace_expand_rhs (name, value, c, quoted,
> pflags, qdollaratp, hasdolla
>     used as the error message to print, otherwise a standard message is
>     printed. */
>  static void
> -parameter_brace_expand_error (name, value)
> +parameter_brace_expand_error (name, value, check_nullness)
>       char *name, *value;
> +     int check_nullness;
>  {
>    WORD_LIST *l;
>    char *temp;
>
>