Skip to content

Vsd value set of pointers #5825

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
merged 5 commits into from
Mar 1, 2021

Conversation

jezhiggins
Copy link
Contributor

This leads on from #5793, separating out value-sets of constants from value-sets of pointers.

Prior to this work, value_set_abstract_objectt did double duty for constants and pointers, contained a certain amount of runtime type checking to do the right thing, doing a decent job for values, but a poor job for pointers. Separating the two out isolates the constant and pointer specific behaviours, immediately increasing clarity and enabling some easy fixes for pointer dereferencing and writing through.

I could have gone further, as the pointer behaviour, while improved, is not complete, and there's still a certain amount of duplication that could be eliminated. However, I believe much of this will be addressed, and actually more easily addressed, in the upcoming working on multiple-dispatch/cross-representation evaluation and merge.

@codecov
Copy link

codecov bot commented Feb 14, 2021

Codecov Report

Merging #5825 (df41607) into develop (e5e8239) will increase coverage by 0.05%.
The diff coverage is 85.20%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5825      +/-   ##
===========================================
+ Coverage    72.85%   72.90%   +0.05%     
===========================================
  Files         1421     1425       +4     
  Lines       154130   154236     +106     
===========================================
+ Hits        112292   112452     +160     
+ Misses       41838    41784      -54     
Impacted Files Coverage Δ
...yses/variable-sensitivity/abstract_environment.cpp 87.42% <ø> (ø)
...ble-sensitivity/constant_pointer_abstract_object.h 100.00% <ø> (ø)
...sensitivity/variable_sensitivity_configuration.cpp 70.37% <0.00%> (ø)
...e-sensitivity/variable_sensitivity_configuration.h 100.00% <ø> (ø)
...riable-sensitivity/variable_sensitivity_domain.cpp 87.11% <ø> (-0.24%) ⬇️
...-sensitivity/variable_sensitivity_object_factory.h 100.00% <ø> (ø)
...e-sensitivity/constant_pointer_abstract_object.cpp 86.51% <66.66%> (ø)
...s/variable-sensitivity/abstract_pointer_object.cpp 76.66% <70.00%> (ø)
...-sensitivity/value_set_pointer_abstract_object.cpp 78.82% <80.72%> (+18.82%) ⬆️
...variable-sensitivity/value_set_abstract_object.cpp 91.71% <90.00%> (+4.70%) ⬆️
... and 62 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 268f886...df41607. Read the comment docs.

@jezhiggins jezhiggins force-pushed the vsd-value-set-of-pointers branch 2 times, most recently from 85160cc to 8342a92 Compare February 15, 2021 08:39
@jezhiggins jezhiggins marked this pull request as ready for review February 15, 2021 09:46
Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

Approve subject to minor corrections.

@@ -2,23 +2,20 @@

Module: analyses variable-sensitivity

Author: Thomas Kiley, [email protected]
Author: Jez Higgins, [email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although the author fields are a bit meaningless in the age of git praise / git blame, we do prefer to have the name of the original author.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a feeling this might be something of a git artefact, but happy to make the changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If these are new files (that git has just decided are moved) then they should have you as the author. If they are moved they should keep the original author.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I can't remember, and as you say not hugely important. However, since it obviously looked odd in the diff, I've made the changes.

@@ -2,84 +2,52 @@

Module: analyses variable-sensitivity

Author: Thomas Kiley, [email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

const exprt &expr,
const abstract_environmentt &environment,
const namespacet &ns);

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the function stays then it would be good if the doxygen stays / gets updated.


Author: Diffblue Ltd.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should remain.

Introduce new base class, abstract_pointer_objectt. It should be,
but is not yet, an abstract (in C++ terms) class as it's not
intended to be directly instantiable.

pointer_abstract_object now renamed two_value_pointer_abstract_objectt.

constant_pointer_abstract_objectt and two_value_pointer_abstract_objectt
are now peers.

Deleted the unused value_set_pointer_abstract_objectt in anticipation of
reintroducing a more functional value-set pointer representation
shortly.
Stripped back value_set_abstract_object now it doesn't
have to deal with pointers.

Pulled out abstract_object_sett as a type not just an
alias. Add behaviour with functions pulled across from
value_set_abstract_objectt and
value_set_pointer_abstract_objectt.
Remove expression_transform and write from value-set of pointers.
Dereference value-set of pointers to a single value
Test uses intervals. Value-sets should also work but we can't write
that test yet as we needs more work on asserting value-set equality.
Tests determinate and indeterminate write through a pointer
@jezhiggins jezhiggins force-pushed the vsd-value-set-of-pointers branch from 8342a92 to df41607 Compare March 1, 2021 08:45
@martin-cs martin-cs merged commit ced2d39 into diffblue:develop Mar 1, 2021
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.

2 participants