Skip to content

Refactor replace_history_parameter #7686

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

tautschnig
Copy link
Collaborator

Please review commit-by-commit. This is drive-by cleanup while reviewing #7672 with the hope to make it easier to read.

  • Each commit message has a non-empty body, explaining why the change was made.
  • 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.

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.07 ⚠️

Comparison is base (ba47c86) 78.55% compared to head (74159df) 78.48%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7686      +/-   ##
===========================================
- Coverage    78.55%   78.48%   -0.07%     
===========================================
  Files         1686     1686              
  Lines       192923   192930       +7     
===========================================
- Hits        151552   151427     -125     
- Misses       41371    41503     +132     
Impacted Files Coverage Δ
...nt/contracts/dynamic-frames/dfcc_instrument_loop.h 66.66% <ø> (ø)
src/goto-instrument/contracts/contracts.cpp 94.82% <100.00%> (+<0.01%) ⬆️
.../contracts/dynamic-frames/dfcc_instrument_loop.cpp 99.62% <100.00%> (-0.01%) ⬇️
src/goto-instrument/contracts/utils.cpp 94.44% <100.00%> (+1.35%) ⬆️
src/goto-instrument/contracts/utils.h 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tautschnig tautschnig marked this pull request as draft April 27, 2023 10:18
@tautschnig
Copy link
Collaborator Author

Will hold back on merging this until after #7541 is merged.

@feliperodri feliperodri added the aws Bugs or features of importance to AWS CBMC users label May 1, 2023
@tautschnig tautschnig force-pushed the cleanup/refactor-replace_history_parameter branch from 7814587 to 6d9d4a9 Compare May 12, 2023 14:34
This function had some in/out parameters that were really only used as
out parameters, and also an undocumented (but crucial) `id` parameter.
Said `id` can only have one of two permitted values, so replace one
function with a parameter by two functions that have said `id` encoded
in the name. Return out-parameters by value rather than via references.
There are no ordering requirements here, so we can benefit from O(1)
amortised cost. Also, the value type is always a `symbol_exprt`, not
just a generic `exprt`.
Reduce the indentation level by doing early returns, and do the map
lookup just once instead of three times.
@tautschnig tautschnig force-pushed the cleanup/refactor-replace_history_parameter branch from 6d9d4a9 to d875666 Compare May 15, 2023 08:08
@tautschnig tautschnig marked this pull request as ready for review May 15, 2023 08:08
The C front-end already performs the very same check.
@tautschnig tautschnig merged commit 8c152bc into diffblue:develop May 15, 2023
@tautschnig tautschnig deleted the cleanup/refactor-replace_history_parameter branch May 15, 2023 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws Bugs or features of importance to AWS CBMC users cleanup Code Contracts Function and loop contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants