Skip to content

Fix use of equality of value_sett::object_mapt #4705

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

Merged

Conversation

romainbrenguier
Copy link
Contributor

This was removed recently in pull request #4694 but it
seems necessary when compiling with clang which would complain about it
not being defined.

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • [na] The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • [na] My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • [na] White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@romainbrenguier romainbrenguier added the Needs TG approval 🦉 Only merge with explicit approval from test-gen maintainers label May 24, 2019
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is a fix for #4699 ? If so, note that in the commit message and add a comment that this is a pointless specialisation of https://github.com/diffblue/cbmc/blob/develop/src/util/reference_counting.h#L185 which we supply to tolerate old broken compilers.

@romainbrenguier romainbrenguier force-pushed the bugfix/object-map-equality branch 3 times, most recently from 8a4fc26 to d3d95fe Compare May 24, 2019 14:26
@romainbrenguier romainbrenguier changed the title Define equality of value_sett::object_mapt Fix use of equality of value_sett::object_mapt May 24, 2019
@romainbrenguier romainbrenguier force-pushed the bugfix/object-map-equality branch from d3d95fe to 194eeef Compare May 24, 2019 16:55
@romainbrenguier romainbrenguier removed In progress Needs TG approval 🦉 Only merge with explicit approval from test-gen maintainers labels May 24, 2019
@romainbrenguier
Copy link
Contributor Author

@smowton

I assume this is a fix for #4699 ? If so, note that in the commit message and add a comment that this is a pointless specialisation of https://github.com/diffblue/cbmc/blob/develop/src/util/reference_counting.h#L185 which we supply to tolerate old broken compilers.

Yes, I have added that to the commit message. I also changed a bit the code so that we don't redefine the equality function.

@smowton
Copy link
Contributor

smowton commented May 24, 2019

Shame to throw away the reference-equal case though? If you're going to inline reference_countingt::operator== then inline the whole thing

@romainbrenguier romainbrenguier force-pushed the bugfix/object-map-equality branch from 194eeef to 8cd09b3 Compare May 24, 2019 17:08
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️
Passed Diffblue compatibility checks (cbmc commit: 8cd09b3).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/113132139

@romainbrenguier
Copy link
Contributor Author

@smowton

Shame to throw away the reference-equal case though? If you're going to inline reference_countingt::operator== then inline the whole thing

this is now fixed

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment noting that this is a copy of (link to reference_countingt::operator==) and noting why this was done

The explicit definition of object_mapt equality was removed recently in
pull request diffblue#4694 but it seems necessary when compiling
with clang which would complain about it not being defined.

Should fix diffblue#4699
@romainbrenguier romainbrenguier force-pushed the bugfix/object-map-equality branch from 8cd09b3 to b27ceb9 Compare May 28, 2019 09:20
@allredj
Copy link
Contributor

allredj commented May 28, 2019

We're fixing something for clang 3.8, but we're not compiling with clang 3.8 on CI. So this (or something similar) will probably break later and we'll waste more time. Should we not add another build on CI?

@romainbrenguier
Copy link
Contributor Author

@allredj

We're fixing something for clang 3.8, but we're not compiling with clang 3.8 on CI. So this (or something similar) will probably break later and we'll waste more time. Should we not add another build on CI?

pull request to add the CI step: #4718

@romainbrenguier romainbrenguier merged commit 9700301 into diffblue:develop May 28, 2019
@romainbrenguier romainbrenguier deleted the bugfix/object-map-equality branch May 28, 2019 10:26
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️
Passed Diffblue compatibility checks (cbmc commit: b27ceb9).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/113404218

@thk123 thk123 mentioned this pull request May 28, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants