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

Remove support for irep_idt-as-std::string #8154

Merged

Conversation

tautschnig
Copy link
Collaborator

While #8040 ensured we were still able to compile the code base with irep_idt typedef'd to std::string (via USE_STD_STRING), experiments at that time also demonstrated that performance is substantially worse, and not all tests would pass.

This commit now removes this untested feature to pave the way for changes that may break dstringt to std::string compatibility.

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

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (69bb2b6) 79.09% compared to head (4d935bf) 79.09%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8154      +/-   ##
===========================================
- Coverage    79.09%   79.09%   -0.01%     
===========================================
  Files         1695     1695              
  Lines       196583   196582       -1     
===========================================
- Hits        155496   155492       -4     
- Misses       41087    41090       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tautschnig tautschnig assigned peterschrammel and TGWDB and unassigned kroening Jan 10, 2024
While diffblue#8040 ensured we were still able to compile the code base with
`irep_idt` typedef'd to `std::string` (via `USE_STD_STRING`),
experiments at that time also demonstrated that performance is
substantially worse, and not all tests would pass.

This commit now removes this untested feature to pave the way for
changes that may break `dstringt` to `std::string` compatibility.
@tautschnig tautschnig force-pushed the cleanup/remove_use_std_string branch from ea2643e to 4d935bf Compare January 11, 2024 15:04
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.

Whatever you think about removing this feature, it should absolutely not be referenced anywhere in analyses/. If anyone wants to veto this patch then these uses still need to be removed.

@tautschnig tautschnig merged commit 2d96b83 into diffblue:develop Feb 5, 2024
@tautschnig tautschnig deleted the cleanup/remove_use_std_string branch February 5, 2024 21:48
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.

5 participants