Skip to content

Generalise get_subexpression_at_offset to extract parts of members #3167

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 31, 2018
Merged

Generalise get_subexpression_at_offset to extract parts of members #3167

merged 1 commit into from
Oct 31, 2018

Conversation

@tautschnig
Copy link
Collaborator

@tautschnig tautschnig commented Oct 14, 2018

This enables re-use in the simplifier and other places. Adding a unit test of
get_subexpression_at_offset.

  • Each commit message has a non-empty body, explaining why the change was made.
  • My contribution is formatted in line with CODING_STANDARD.md.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • 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).
  • n/a My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@tautschnig tautschnig self-assigned this Oct 14, 2018
martin-cs
martin-cs reviewed Oct 15, 2018
View reviewed changes
src/pointer-analysis/value_set_dereference.cpp Outdated
// try to build a member/index expression - do not use byte_extract
exprt subexpr = get_subexpression_at_offset(
root_object_subexpression, o.offset(), dereference_type, ns);
root_object_subexpression, o.offset(), dereference_type, "", ns);
Copy link
Collaborator

@martin-cs martin-cs Oct 15, 2018

Choose a reason for hiding this comment

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

Having an empty string literal, interpreted as an irep ID, to block particular functionality (descending within objects if I understand correctly) does not seem like the most intuitive of designs. I can't immediately suggest better though.

@tautschnig tautschnig force-pushed the get_subexpr-parts-of-members branch from 0ab3688 to 827094b Compare October 15, 2018 18:24
@tautschnig
Copy link
Collaborator Author

tautschnig commented Oct 15, 2018

Marking do-not-merge as this depends on #3184.

@tautschnig tautschnig mentioned this pull request Oct 15, 2018
Merged
3 tasks
@tautschnig tautschnig force-pushed the get_subexpr-parts-of-members branch 2 times, most recently from 8d7fbad to dc227a0 Compare October 29, 2018 20:32
allredj
allredj reviewed Oct 29, 2018
View reviewed changes
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.

🚫
This PR failed Diffblue compatibility checks (cbmc commit: 8d7fbad).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/89566003
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

  • the cbmc commit has disappeared in the mean time (e.g. in a force-push)
  • the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

@tautschnig tautschnig force-pushed the get_subexpr-parts-of-members branch from dc227a0 to aa683aa Compare October 29, 2018 21:09
@tautschnig
Copy link
Collaborator Author

tautschnig commented Oct 29, 2018

Dependencies have been merged, this is now ready for review.

allredj
allredj reviewed Oct 29, 2018
View reviewed changes
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.

🚫
This PR failed Diffblue compatibility checks (cbmc commit: aa683aa).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/89578879
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

  • the cbmc commit has disappeared in the mean time (e.g. in a force-push)
  • the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

@kroening kroening removed their assignment Oct 30, 2018
smowton
smowton approved these changes Oct 30, 2018
View reviewed changes
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.

Recommend name your variables after units given bytes and bits are both used in the same function; otherwise looks good

@tautschnig
Generalise get_subexpression_at_offset to extract parts of members
2ff4d2f 
This enables re-use in the simplifier and other places. Adding a unit test of
get_subexpression_at_offset.
@tautschnig tautschnig force-pushed the get_subexpr-parts-of-members branch from aa683aa to 2ff4d2f Compare October 31, 2018 13:12
@tautschnig tautschnig merged commit b16d247 into diffblue:develop Oct 31, 2018
@tautschnig tautschnig deleted the get_subexpr-parts-of-members branch October 31, 2018 13:55
allredj
allredj reviewed Oct 31, 2018
View reviewed changes
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: 2ff4d2f).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/89808073

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martin-cs martin-cs martin-cs left review comments

@kroening kroening kroening approved these changes

@chrisr-diffblue chrisr-diffblue Awaiting requested review from chrisr-diffblue

@peterschrammel peterschrammel Awaiting requested review from peterschrammel peterschrammel is a code owner

@pkesseli pkesseli Awaiting requested review from pkesseli

@cesaro cesaro Awaiting requested review from cesaro

@cristina-david cristina-david Awaiting requested review from cristina-david

@NathanJPhillips NathanJPhillips Awaiting requested review from NathanJPhillips

@thk123 thk123 Awaiting requested review from thk123

+2 more reviewers

@allredj allredj allredj left review comments

@smowton smowton smowton approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@smowton smowton

@peterschrammel peterschrammel

@martin-cs martin-cs

@chrisr-diffblue chrisr-diffblue

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tautschnig @smowton @kroening @martin-cs @allredj @peterschrammel @pkesseli @chrisr-diffblue