checksrc: ASSIGNWITHINCONDITION check broken #4683
Comments
The regexp looking for assignments within conditions was too greedy
and matched a too long string in the case of multiple conditionals
on the same line. This is basically only a problem in single line
macros, and the code which exemplified this was essentially:
do { if((x) != NULL) { x = NULL; } } while(0)
..where the final parenthesis of while(0) matched the regexp, and
the legal assignment in the block triggered the warning. Fix by
making the regexp less greedy by matching for the tell-tale signs
of the if statement ending.
Also remove the one occurrence where the warning was disabled due
to a construction like the above, where the warning didn't apply
when fixed.
Closes #4647
Reviewed-by: Daniel Stenberg <daniel@haxx.se>
|
I propose we revert the change and instead fix the macros so that we don't do multiple expressions like that one the same line. We discourage that in the code style document already! |
|
Regardless of what we do, we should probably change the macros to match the mandated style. The bigger question is, should we at all attempt this since we cannot test the full expression due to the single-line mode of checksrc? Maybe enabling the MSVC Level4 assignment-within-conditional warning in a CI build is a better catch-all (C4706)? |
|
I'd rather have something that works somewhat than not at all. I also like that we can disable checksrc warnings in specific instances. Anyway though I don't think we should be doing assignments in conditionals ever so it seems appropriate to enable that MSVC warning if it's not already. |
|
Having an assignment-within-conditional warning enabled in compiler should only be a good thing for us! |
... even for macros Reported-by: Jay Satiro Fixes #4683
I did this
Recent changes in ba82673 to improve on the ASSIGNWITHINCONDITION regex check actually broke it further. Some suggestions have been made in the linked to discussion. I'd prefer we move the discussion here for tracking purposes.
To test regexes and see colored results (which you may find very helpful) I suggest https://regex101.com in PCRE mode which is pretty close to perl.
I expected the following
A single line if(expression) should always be checked by ASSIGNWITHINCONDITION.
curl/libcurl version
master 147fa06 2019-12-07
The text was updated successfully, but these errors were encountered: