lnav icon indicating copy to clipboard operation
lnav copied to clipboard

lnav source is replete with signedness mismatches

Open he32 opened this issue 2 years ago • 0 comments

lnav version v0.11.1

Describe the bug The lnav source code is littered with signedness mismatches. This appears to stem from an infatuation to use size_t-typed variables as loop variables, and subsequently either asserting or comparing said loop variable with a plain signed int expression. This becomes evident if you build lnav with the -Wsign-compare option. A couple of examples:

pcre2pp.cc: In member function 'std::string lnav::pcre2pp::code::replace(string_fragment, const char*) const':
pcre2pp.cc:300:46: warning: comparison of integer expressions of different signedness: 'int' and 'size_t' {aka 'unsigned int'} [-Wsign-compare]
  300 |                     } else if (capture_index > this->get_capture_count()) {
      |                                ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~

and:

is_utf8.cc: In function 'utf8_scan_result is_utf8(const unsigned char*, size_t, const char**, int*, nonstd::optional_lite::optional<unsigned char>)':
is_utf8.cc:74:14: warning: comparison of integer expressions of different signedness: 'ssize_t' {aka 'int'} and 'size_t' {aka 'unsigned int'} [-Wsign-compare]
   74 |     while (i < len) {
      |            ~~^~~~~

The build log for lnav ends up with no less than 140 such warnings.

To Reproduce Build the program with the -Wsign-compare option.

While it can possibly be argued that the values which are likely to be compared would not make this a problem, but such justification would need to be made for all the 140 instances. I think that this lays down a fertile ground for bugs to creep into the code, and ignoring the warnings is probably not a good idea in general. What's wrong with plain int loop variables?

he32 avatar May 24 '23 22:05 he32