Skip to content

Remove long deprecated methods #4427

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 7 commits into from
Mar 26, 2019
Merged

Remove long deprecated methods #4427

merged 7 commits into from
Mar 26, 2019

Conversation

tautschnig
Copy link
Collaborator

The first commit is #4423, the other commits are new. This is a cleanup of all entities that have been marked deprecated for more than 6 months except for

  1. Cleanup already in pull requests and
  2. the procedures in the string solver - they have been marked as deprecated since 2017, but actually removing them (where this is even possible, there are still in-tree uses) causes tests to fail.
  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a 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).
  • 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 Mar 23, 2019
martin-cs
martin-cs approved these changes Mar 23, 2019
View reviewed changes
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.

"One of my most productive days was throwing away 1,000 lines of code."

@tautschnig tautschnig force-pushed the remove-long-deprecated-methods branch from 9eb6015 to b8c5745 Compare March 23, 2019 23:14
@tautschnig tautschnig changed the title Remove long deprecated methods [depends-on: #4423] Remove long deprecated methods Mar 23, 2019
allredj
allredj reviewed Mar 23, 2019
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: 9eb6015).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105559553
Status will be re-evaluated on next push.
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 compatibility was already broken by an earlier merge.

allredj
allredj reviewed Mar 24, 2019
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: b8c5745).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105560177
Status will be re-evaluated on next push.
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 compatibility was already broken by an earlier merge.

@tautschnig tautschnig force-pushed the remove-long-deprecated-methods branch from b8c5745 to 5f332e4 Compare March 24, 2019 07:14
allredj
allredj reviewed Mar 24, 2019
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: 5f332e4).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105570026
Status will be re-evaluated on next push.
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 compatibility was already broken by an earlier merge.

@kroening
Copy link
Member

My only problem with this is the removal of

side_effect_exprt(const irep_idt &statement, const typet &_type)

in favour of a variant with source_locationt. We don't do that agenda consistently for all expressions, and it's unclear why that one is singled out.

@tautschnig
Copy link
Collaborator Author

My only problem with this is the removal of side_effect_exprt(const irep_idt &statement, const typet &_type) [...]

Maybe it should just never have been deprecated in the first place? I have no issue not removing that constructor, I just did a rather mechanic removal of long-deprecated constructors. Please advise.

@tautschnig tautschnig force-pushed the remove-long-deprecated-methods branch from 5f332e4 to 6409b21 Compare March 26, 2019 07:42
@tautschnig
Copy link
Collaborator Author

I have now left those constructors that only differ from non-deprecated ones by a source_locationt parameter in the code base. Will merge once CI passes, but we still need to consider whether the deprecation markers should be removed.

@tautschnig tautschnig assigned tautschnig and unassigned kroening Mar 26, 2019
allredj
allredj reviewed Mar 26, 2019
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: 6409b21).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105802020
Status will be re-evaluated on next push.
Common spurious failures include: 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); compatibility was already broken by an earlier merge.

@tautschnig tautschnig added the Needs TG approval 🦉 Only merge with explicit approval from test-gen maintainers label Mar 26, 2019
@thk123
Copy link
Contributor

thk123 commented Mar 26, 2019

Updated TG - rebasing this should get the Joel bot to pass

@tautschnig
Remove deprecated side_effect_expr*t constructors
865242c 
These have been deprecated from more than six months.
@tautschnig
Remove deprecated instrument_cover_goals
b369844 
It has been deprecated for more than 12 months.
@tautschnig
Remove deprecated SSA_stept::output(namespacet, std::ostream)
2c93618 
It has been deprecated for 11 months (but actually still had one user,
which has now been fixed).
@tautschnig
Remove deprecated integer2{ulong,size_t,unsigned}
5c2ffd3 
They have been deprecated since 2017.
@tautschnig
Remove deprecated code*t constructors
00713d6 
They have been deprecated from 9 months.
@tautschnig
Remove deprecated nil_typet
70e3c60 
It has been deprecated for 7 months.
@tautschnig
Remove deprecated code_typet constructor
d794d17 
It has been deprecated for 9 months.
@tautschnig tautschnig force-pushed the remove-long-deprecated-methods branch from 6409b21 to d794d17 Compare March 26, 2019 14:17
@tautschnig tautschnig removed the Needs TG approval 🦉 Only merge with explicit approval from test-gen maintainers label Mar 26, 2019
@tautschnig tautschnig merged commit 963b9af into diffblue:develop Mar 26, 2019
@tautschnig tautschnig deleted the remove-long-deprecated-methods branch March 26, 2019 15:15
allredj
allredj reviewed Mar 26, 2019
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: d794d17).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105859717
Status will be re-evaluated on next push.
Common spurious failures include: 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); compatibility was already broken by an earlier merge.

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

@allredj allredj allredj left review comments

@smowton smowton smowton approved these changes

@peterschrammel peterschrammel peterschrammel approved these changes

@martin-cs martin-cs martin-cs approved these changes

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

@danpoe danpoe Awaiting requested review from danpoe

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue Awaiting requested review from hannes-steffenhagen-diffblue

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

@pkesseli pkesseli Awaiting requested review from pkesseli

@romainbrenguier romainbrenguier Awaiting requested review from romainbrenguier

Assignees

@tautschnig tautschnig

Labels
cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tautschnig @kroening @thk123 @smowton @peterschrammel @martin-cs @allredj