Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

checksrc: ASSIGNWITHINCONDITION check broken #4683

Closed
jay opened this issue Dec 7, 2019 · 4 comments
Closed

checksrc: ASSIGNWITHINCONDITION check broken #4683

jay opened this issue Dec 7, 2019 · 4 comments
Labels

Comments

@jay
Copy link
Member

@jay jay commented Dec 7, 2019

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

@jay jay added the build label Dec 7, 2019
jay referenced this issue Dec 7, 2019
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>
@bagder
Copy link
Member

@bagder bagder commented Dec 10, 2019

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!

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Dec 10, 2019

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)?

@jay
Copy link
Member Author

@jay jay commented Dec 10, 2019

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.

@bagder
Copy link
Member

@bagder bagder commented Dec 11, 2019

Having an assignment-within-conditional warning enabled in compiler should only be a good thing for us!

bagder added a commit that referenced this issue Dec 16, 2019
... even for macros

Reported-by: Jay Satiro
Fixes #4683
bagder added a commit that referenced this issue Dec 17, 2019
This reverts commit ba82673.

Bug: #4683
@bagder bagder closed this in bdb5b6d Dec 17, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.