bash-4.2: poor order of conditionals

[David Binderman]

Hello there,

I just ran the static analysis tool cppcheck over the source
code of bash-4.2

It said

[bash-4.2/builtins/mkbuiltins.c:554]: (style) Array index i is used before 
limits check

The source code is

  while (buffer[i] != '\n' && i < file_size)
i++;

You might be better off with

  while (i < file_size && buffer[i] != '\n')
i++;


Regards

David Binderman

  

bash-4.3 bug report

[David Binderman]

Hello there,

 [bind.c:2238]: (style) Array index 'j' is used before limits check.

Source code is

  for (j = 0; invokers[j] && j < 5; j++)

Suggest new code

  for (j = 0; (j < 5) && (invokers[j] != NULL); j++)

Regards

David Binderman


  

RE: bash-4.3 bug report

[David Binderman]

Hello there,


> But my point remains to the original poster: a patch
> without justification is unlikely to be applied. Document WHY you think
> the existing code is a bug, not just HOW to fix it, for your patch to be
> usefully considered.

Standard software engineering practice is to look before leaping.
This means always check the array index before use.
The static analyser implements that standard practice.

The code in question, independent of whether it works ok or not,
does it's work in a non-standard way when the standard way
is easy to achieve and has some possible benefits for robustness,
as well as being easier on the eye to the experienced code reviewer.

Anyone experienced looking at the code will always need to examine it
more closely to find out why it's a good idea in this case to use an array
index and *then* sanity check it's value.


Regards

David Binderman