Skip to content

remove symbol_typet [blocks: #4056] #4264

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 3 commits into from
Feb 28, 2019
Merged

remove symbol_typet [blocks: #4056] #4264

merged 3 commits into from
Feb 28, 2019

Conversation

kroening
Copy link
Member

The type is not created anymore.

  • 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.

@smowton
Copy link
Contributor

smowton commented Feb 25, 2019

CI failures are genuine (residual uses in cpp_typecheck_constructor.cpp and Doxy references in README)

@tautschnig tautschnig changed the title remove symbol_typet remove symbol_typet [blocks: #4056] Feb 27, 2019
@tautschnig tautschnig mentioned this pull request Feb 27, 2019
Closed
4 tasks
@tautschnig
Fix remaining uses of ID_symbol_type in dump-c
6dbdd19 
There were still a few left-overs that had not been converted to struct/union
tags.
@tautschnig
Copy link
Collaborator

I have added two commits that fix remaining uses of symbol_typet. They are bug fixes, but we either don't have tests exercising those code paths or those code paths are actually unreachable. In the interest of moving on to the PRs that this one (transitively) blocks I hope it's acceptable to people that we don't actually spend time on adding tests for such code right now. (We should be doing CMake builds with coverage analysis enabled...)

tautschnig
tautschnig approved these changes Feb 27, 2019
View reviewed changes
Copy link
Collaborator

@tautschnig tautschnig left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving of the overall goal. Two of the commits are mine, so I shouldn't be the one to rely on for approvals anyway...

tautschnig and others added 2 commits February 27, 2019 23:43
@tautschnig
Handle struct/union/enum tag types instead of symbol types
30129ba 
These are left overs that had been missed in prior migration to tag types.
Technically these are bug fixes, but it seems we do not have tests for these
code paths.
@tautschnig
Remove symbol_typet
44858f2 
All front-ends now generate tag types, and tests for ID_symbol_type have become
dead branches/dead code.
@tautschnig tautschnig assigned smowton and unassigned kroening Feb 27, 2019
allredj
allredj reviewed Feb 28, 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: 44858f2).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/102589932
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 assigned chrisr-diffblue and unassigned smowton and thk123 Feb 28, 2019
@allredj
Copy link
Contributor

allredj commented Feb 28, 2019

https://github.com/diffblue/test-gen/pull/3101 removes some stale references to symbol_typet. No need to wait for it though.

@tautschnig tautschnig merged commit ca54005 into develop Feb 28, 2019
@tautschnig tautschnig deleted the cleanout-symbol_type branch February 28, 2019 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smowton smowton smowton approved these changes

@tautschnig tautschnig tautschnig approved these changes

@allredj allredj allredj approved these changes

@chrisr-diffblue chrisr-diffblue chrisr-diffblue approved these changes

@danpoe danpoe Awaiting requested review from danpoe

@martin-cs martin-cs Awaiting requested review from martin-cs martin-cs is a code owner

@owen-mc-diffblue owen-mc-diffblue Awaiting requested review from owen-mc-diffblue

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

@pkesseli pkesseli Awaiting requested review from pkesseli

@romainbrenguier romainbrenguier Awaiting requested review from romainbrenguier

@thk123 thk123 Awaiting requested review from thk123

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

Assignees

@peterschrammel peterschrammel

@danpoe danpoe

@chrisr-diffblue chrisr-diffblue

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue

Labels
blocker bugfix cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@kroening @smowton @tautschnig @allredj @chrisr-diffblue @thk123 @peterschrammel @danpoe @hannes-steffenhagen-diffblue