[bug #60639] src/utils/addftinfo/addftinfo.cpp:119: double increment ?

[G. Branden Robinson]

Follow-up Comment #5, bug #60639 (project groff):

Thanks for the quick analysis, Ingo.

[comment #1 comment #1:]
> The for loops were revised (by original author James Clark) in 1995--not to
add redundant incrementations, but rather to dumb the code down to ISO C90
from what was then a GCC extension.

Ironically, as part of my efforts to improve diagnostic messages from the
groff system, I added a loop-scoped index variable as part of commit
2a4c8d7475f61f9056c0a25b5ae68843e3c7fbe7 (29 April 2020), just like the kind
James Clark got rid of in 1995.

[comment #4 comment #4:]
> > I think i += 2 in the last part of the for loop specifier
> > might make the programmer's original intent more clear.
> 
> Purely a matter of style, there are arguments both ways.
> The existing code puts each increment close to the code
> handling the respective argument, and your suggestion
> would increase the risk of causing bugs when introducing
> the first option that does not take an argument.

I think this point is relatively weak given addftinfo's age and purpose.  I
don't think anyone expects exciting developments in the AT&T
device-independent troff font format that our utility will need to adapt to. 
But usually, yes, it's a much stronger consideration.

> Changing code just because a compiler has suspicions is not a
> good idea - unless the code is indeed unambiguously bad style
> or buggy and the compiler merely finds a real problem.  But the
> only question that ever matters is "is there a real problem
> with the code" and never "does the compiler complain".  The
> compiler is only a tool and must never be used blindly.

This, I agree with as well.

It's a little disappointing that Clang wasn't bright enough to analyze the
code paths through the for loop and determine that at least one of them does
NOT result in the double increment.


   115      for (j = 0;; j++) {
   116        if (j >= sizeof(param_table)/sizeof(param_table[0]))
   117          fatal("parameter '%1' not recognized", argv[i] + 1);
   118        if (strcmp(param_table[j].name, argv[i] + 1) == 0)
   119          break;
   120      }


Also, I don't think lines 112-113 work as intended.


$ addftinfo -asc-height -body-depth 60
usage: addftinfo [-v] [-param value] ... resolution unitwidth font
$ addftinfo -body-depth 60 -asc-depth
usage: addftinfo [-v] [-param value] ... resolution unitwidth font


Our forthcoming release improves the usage message (IMO) but not this
validation step.


$ ./build/addftinfo -asc-height -body-depth 60
usage: ./build/addftinfo [-asc-height N] [-body-depth N] [-body-height N]
[-cap-height N] [-comma-depth N] [-desc-depth N] [-fig-height N] [-x-height N]
RESOLUTION UNIT-WIDTH FONT
usage: ./build/addftinfo -v
usage: ./build/addftinfo --version
$ ./build/addftinfo -body-depth 60 -asc-depth
usage: ./build/addftinfo [-asc-height N] [-body-depth N] [-body-height N]
[-cap-height N] [-comma-depth N] [-desc-depth N] [-fig-height N] [-x-height N]
RESOLUTION UNIT-WIDTH FONT
usage: ./build/addftinfo -v
usage: ./build/addftinfo --version


Maybe I'll fix this for grins.

All that said, I'm open to considering a specialized comment or pragma push
that shuts up this compiler warning.  If David's seen it--more people will.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?60639>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/