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

Assignment in assertions #701

Closed
kkarbowiak opened this issue Apr 22, 2022 · 2 comments
Closed

Assignment in assertions #701

kkarbowiak opened this issue Apr 22, 2022 · 2 comments

Comments

@kkarbowiak
Copy link
Contributor

It looks like there are two cases of performing an assignment instead of equality check inside assertions:

  • algorithms/validation/choose_ETA.cpp:931:
assert(current_delta_rank = nb_var + 1);
  • algorithms/validation/choose_ETA.cpp:999:
assert(current_delta_rank = nb_var + 1);

I stumbled upon those thanks to compiler warnigs (compilation using g++ 11.2.0):

algorithms/validation/choose_ETA.cpp: In function ‘vroom::Route vroom::validation::choose_ETA(const vroom::Input&, unsigned int, const std::vector<vroom::VehicleStep>&)’:
algorithms/validation/choose_ETA.cpp:931:29: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
  931 |   assert(current_delta_rank = nb_var + 1);
      |          ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
algorithms/validation/choose_ETA.cpp:999:29: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
  999 |   assert(current_delta_rank = nb_var + 1);
      |          ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~

Both current_delta_rank and nb_var are of type unsigned, and the assertions can only fail in case of wrap-around. Is this intended? I guess not, if only for the side effects of the assignment missing if compiling with NDEBUG.

I can send a PR that changes assignment to equality check, but I'm afraid I have no means to test this properly.

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 22, 2022

Good catch, those are indeed intended to be regular equality checks, not assignments. Please submit a PR for this if you can, I'll handle the tests.

@jcoupey
Copy link
Collaborator

jcoupey commented Apr 25, 2022

Fixed in #702

@jcoupey jcoupey closed this as completed Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants