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

Removed the dynamic_object_exprt's instance() #716

Merged

Conversation

mariusmc92
Copy link
Contributor

@mariusmc92 mariusmc92 commented Mar 28, 2017

Removed the dynamic_object_exprt's instance() methods and
replaced the usages using set_location_number().

Added the dynamic_object_id_sett data structure into the
value_sett class. It is used as expr_sett but only for
dynamic-objects.

@mariusmc92 mariusmc92 force-pushed the cleanup/type-safe-dynamic-object branch from 1ffc42b to 8eadf47 Compare March 28, 2017 16:10
Copy link
Contributor

@NathanJPhillips NathanJPhillips left a comment

Choose a reason for hiding this comment

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

Looks right to me.

@NathanJPhillips
Copy link
Contributor

This improves type safety and prepares the way for being able to access the location number as a location number (rather than an opaque string) without having to check the types whenever it is accessed.
This will provide a better basis for other extensions that will be required to dynamic object in SSS and be useful when doing other operations on dynamic objects as part of Marius' later work.

@tautschnig
Copy link
Collaborator

I'm all in for the encapsulation part of this patch, but how is the renaming set_instance -> set_location_number justified?

@NathanJPhillips
Copy link
Contributor

All previous usage of this field for stuffing any other information in has been removed and replaced with better typed storage. It now exclusively holds the location number of the malloc that creates this DO. A more descriptive name is hence justified.

@mariusmc92 mariusmc92 force-pushed the cleanup/type-safe-dynamic-object branch from 8eadf47 to 2a5e6c6 Compare March 29, 2017 09:26
@tautschnig
Copy link
Collaborator

tautschnig commented Mar 29, 2017

It now exclusively holds the location number of the malloc that creates this DO. A more descriptive name is hence justified.

Unless I'm misreading the code: pointer-analysis/value_set_fi*.cpp seem to be encoding a different bit of information via bit manipulation?

@smowton
Copy link
Contributor

smowton commented Mar 29, 2017

Worth checking with Daniel, but I don't see why the _fi variants can't assume that location_number is a sufficient identifier?

@smowton
Copy link
Contributor

smowton commented Mar 29, 2017

(the from_index variable is always set from a goto_programt::instructiont::location_number, which are globally unique, so I suspect this was written before that was true, and/or by someone who wasn't sure if it was true)

@tautschnig
Copy link
Collaborator

tautschnig commented Mar 30, 2017

@smowton Note that these analyses are context-sensitive (as opposed to the plain value_sett), and from_function encodes the function that is the caller (one level of context). The use of instance thus seems more appropriate as is generalises the concept of a location-bound expression. I would thus maintain that renaming is not appropriate.

Removed the dynamic_object_exprt's instance() methods and
replaced its usages with set_instance() and get_instance().

Added the dynamic_object_id_sett data structure into the
value_sett class. It is used as expr_sett but only for
dynamic-objects.
@mariusmc92 mariusmc92 force-pushed the cleanup/type-safe-dynamic-object branch from f158253 to 66481ab Compare March 30, 2017 13:53
Copy link
Contributor

@NathanJPhillips NathanJPhillips left a comment

Choose a reason for hiding this comment

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

Changes look fine to me.

@peterschrammel
Copy link
Member

@kroening, that looks ready to go.

@kroening kroening merged commit 384bd7c into diffblue:master Apr 3, 2017
@mariusmc92 mariusmc92 deleted the cleanup/type-safe-dynamic-object branch April 3, 2017 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants