Skip to content

Variable sensitivity object factory rework #5610

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

Conversation

jezhiggins
Copy link
Contributor

This PR removes the variable_sensitivity_object_factoryt static instance.

Now, a configured variable_sensitivity_object_factoryt instance is passed into the variable_sensitivity_domain_factoryt, which in turn passes down into the variable_sensitivity_domaint objects it creates. Those pass the object factory to their abstract_environments and, finally, they use the factory to
build the objects they need.

The variable_sensitivity_object_factoryt are created and passed as shared pointers.

Plugged it into the vsd analysis. This is going to be the place where we
can hook in the appropriately configured variable_sensitivity_object_factory.
Add temporary default constructor until we can sort out derived classes
Switch variable_sensitivity_domain_factoryt to an explicit template
specialisation. Without this, we get link errors from the ait
default constructor. This is ever used by when
variable_sensitivity_domaint, but still get generated.
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.

Could be merged as-is. I think the simplification of avoiding ai_domain_factory_default_constructort is worth doing but I won't insist on it.

@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #5610 (dfb0e0d) into develop (bd6acb6) will increase coverage by 0.00%.
The diff coverage is 92.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5610   +/-   ##
========================================
  Coverage    69.33%   69.33%           
========================================
  Files         1242     1242           
  Lines       100405   100416   +11     
========================================
+ Hits         69612    69623   +11     
  Misses       30793    30793           
Flag Coverage Δ
cproversmt2 43.09% <ø> (ø)
regression 66.23% <92.00%> (+<0.01%) ⬆️
unit 32.28% <40.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...lyses/variable-sensitivity/abstract_enviroment.cpp 87.42% <ø> (ø)
...ensitivity/variable_sensitivity_object_factory.cpp 92.72% <ø> (-0.38%) ⬇️
src/goto-analyzer/goto_analyzer_parse_options.cpp 73.35% <75.00%> (+0.07%) ⬆️
...nalyses/variable-sensitivity/abstract_enviroment.h 100.00% <100.00%> (ø)
...sitivity/variable_sensitivity_dependence_graph.cpp 80.21% <100.00%> (+0.10%) ⬆️
...ensitivity/variable_sensitivity_dependence_graph.h 83.33% <100.00%> (+0.40%) ⬆️
...variable-sensitivity/variable_sensitivity_domain.h 100.00% <100.00%> (ø)
...-sensitivity/variable_sensitivity_object_factory.h 97.56% <100.00%> (ø)
...riable-sensitivity/variable_sensitivity_domain.cpp 86.82% <0.00%> (+0.10%) ⬆️
... and 1 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 bd6acb6...dfb0e0d. Read the comment docs.

@tautschnig
Copy link
Collaborator

I was almost going to hit merge on this one, but I'd like to ask for the churn in those commits to be cleaned up first. There are several changes in later commits that just fix code only introduced in this PR. Also, it would be nice if each commit did still fully compile, i.e., updates to the unit test should be merged into the commit that breaks the unit test.

@jezhiggins jezhiggins force-pushed the variable-sensitivity-object-factory-rework branch from aba885c to f7c513c Compare November 19, 2020 10:00
@jezhiggins
Copy link
Contributor Author

@tautschnig I've cleaned up the history around those last steps - you're right about the unit tests and that was an oversight on my part. I'm happy to do more, but I'd like to keep the earlier history as is because collapsing that feels like it makes the change a bit too magical - it was like this, now it's like that - when it was a little bit of a voyage of discovery.

jezhiggins and others added 4 commits November 19, 2020 10:23
Pass variable_sensitivity_object_factoryt_ptr to abstract_environmenti
constructor, which uses it to create the domain objects.
Update unit tests now static variable_sensitivity_object_factoryt is gone
Remove redundant forward declaration
@jezhiggins jezhiggins force-pushed the variable-sensitivity-object-factory-rework branch from f7c513c to dfb0e0d Compare November 19, 2020 10:24
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.

Thanks for the fixes.

@martin-cs martin-cs merged commit 60ffbcd into diffblue:develop Nov 19, 2020
@jezhiggins jezhiggins deleted the variable-sensitivity-object-factory-rework branch November 19, 2020 13:14
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.

3 participants