Suppress some minor analysis warnings #593
Conversation
| single-char opcodes. */ | ||
|
|
||
| reqvary = (repeat_min == repeat_max)? 0 : REQ_VARY; | ||
| op_type = 0; |
There was a problem hiding this comment.
Clang is correct here. The assignment to op_type is provably a dead assignment. What's more... I reckon it hurts rather than helps, because if there's a branch in the code that should be assigning to op_type but forgets, this would suppress warnings about use-of-uninitialised!
| buffer[0] = new_start; | ||
| buffer[1] = new_end; | ||
| buffer += 2; | ||
| (void)buffer; |
There was a problem hiding this comment.
We have some clearly useless assignments here, but they're OK to keep, because they ensure the buffer is correctly tracked if someone extends the function later. Easy suppression.
| { | ||
| allow_zero = TRUE; | ||
| codevalue = *(++code); /* Codevalue will be one of above BRAs */ | ||
| ++code; /* The following opcode will be one of the above BRAs */ |
There was a problem hiding this comment.
Clang is correct. We are writing a value to codevalue which is provably never used.
src/pcre2grep.c
Outdated
| } | ||
|
|
||
| while ((patlen = sizeof(buffer)) && read_pattern(buffer, &patlen, f)) | ||
| while ((patlen = sizeof(buffer), read_pattern(buffer, &patlen, f))) |
There was a problem hiding this comment.
Stupid coverity. It seems unable to reason about the (patlen = sizeof(buffer)) && expression, which assigns to patlen, and then unconditionally continues (because sizeof(buffer) is always > 0).
There was a problem hiding this comment.
This is ikely going to add a -Wcomma warning though and the original code was clearer about intention.
isn't there a pragma or other way to suppress this from coverity without affecting code quality?
There was a problem hiding this comment.
-Wcomma allows assignments on the LHS of a comma operator, so this should be accepted.
I'm not sure the original code was clearer anyway. We're not logically doing an &&: we want to do something with side-effects on the left, ignore whatever it evaluates to, and then execute the right.
There was a problem hiding this comment.
I would rewrite it to while (TRUE), set variable, break if read_pattern fails. This is not a nice code.
There was a problem hiding this comment.
Good point; I've done Zoltan's improvement.
| ret = (int)len; | ||
| } | ||
|
|
||
| PCRE2_ASSERT(len > 0 || preg != NULL); |
There was a problem hiding this comment.
Adding an assertion here lets Clang reason about when preg is NULL. There was no bug, but static analysis wasn't able to work out that checking len was equivalent to checking preg != NULL.
src/pcre2grep.c
Outdated
| } | ||
|
|
||
| while ((patlen = sizeof(buffer)) && read_pattern(buffer, &patlen, f)) | ||
| while ((patlen = sizeof(buffer), read_pattern(buffer, &patlen, f))) |
There was a problem hiding this comment.
This is ikely going to add a -Wcomma warning though and the original code was clearer about intention.
isn't there a pragma or other way to suppress this from coverity without affecting code quality?
NWilson
force-pushed
the
user/niwilson/minor-warnings
branch
from
f7f8679 to
6d300e4
Compare
NWilson
force-pushed
the
user/niwilson/minor-warnings
branch
from
6d300e4 to
b879592
Compare
|
@carenas are the changes ok? |
|
I think it's OK to merge. Carlo's only request was that there (might be, unconfirmed) a |
|
Apologies for the delay, validating the changes was indeed made more difficult by the rebasing of these changes, but it seems that another concern I had might had also sneaked in. It was previously a regression that broke building in AIX with xlc, because it will falsely recognize the compiler as supporting a builtin that it doesn't have because the detection code was optimized out because it was deemed uneeded and without side effects. |
|
got an AIX 7.1 system to validate, and seems my concerns were unfunded, eventhough there might be still problems with |
3 participants
One of these appears in Coverity's dashboard; the rest are from clang-scan.
See #576