Skip to content

Conversation

tautschnig
Copy link
Collaborator

This should be set via constructors. Remove bmc_util/setup_symex as all work is done through constructors.

Keeping as draft until #8643 has been merged (which is the first commit in this PR).

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

Copy link

codecov bot commented May 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.39%. Comparing base (6c3978f) to head (921218f).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8646      +/-   ##
===========================================
- Coverage    80.39%   80.39%   -0.01%     
===========================================
  Files         1688     1688              
  Lines       207411   207405       -6     
  Branches        73       73              
===========================================
- Hits        166757   166750       -7     
- Misses       40654    40655       +1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tautschnig tautschnig force-pushed the language_mode-protected branch from b50279c to 763f4da Compare June 25, 2025 07:37
@tautschnig tautschnig changed the title Make goto_symext::language_mode protected Make goto_symext::language_mode protected [depends-on: 8643] Jul 28, 2025
@tautschnig tautschnig force-pushed the language_mode-protected branch from 763f4da to 2cbff34 Compare September 12, 2025 11:39
@tautschnig tautschnig marked this pull request as ready for review September 12, 2025 11:40
@tautschnig tautschnig changed the title Make goto_symext::language_mode protected [depends-on: 8643] Make goto_symext::language_mode protected Sep 12, 2025
@tautschnig
Copy link
Collaborator Author

Requires #8711 for CI to pass.

This should be set via constructors. Remove bmc_util/setup_symex as all
work is done through constructors.
@tautschnig tautschnig force-pushed the language_mode-protected branch from 2cbff34 to 921218f Compare September 12, 2025 18:27
const symex_target_equationt &equation);

virtual void setup_symex(symex_bmct &symex);
virtual void setup_symex(symex_bmct &symex)
Copy link
Member

Choose a reason for hiding this comment

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

Is this even called anywhere now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's called from initialize_worklist and resume_path in this class. Which is also why this needs to be kept: it will be called more than once in the single-path case. Yet the only actual code being executed is java_setup_symex, and I am wondering whether this needs to be re-done per path, or instead could be done just once when creating symex_bmct?

Copy link
Member

Choose a reason for hiding this comment

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

java_setup_symex just installs a loop_unwind_handler. This should be done only once when creating symex_bmct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, when writing my earlier comment I had failed to consider that initialize_worklist and resume_path actually do create a fresh symex_bmct instance each time, so each time we do need to invoke java_setup_symex. So I'd argue the code is actually ok as it is?!

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes.

unwindset.parse_unwind(options.get_option("unwind"));
unwindset.parse_unwindset(
options.get_list_option("unwindset"), goto_model, ui_message_handler);
setup_symex(symex, ns, ui_message_handler);
Copy link
Member

Choose a reason for hiding this comment

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

If you remove this then the virtual function won't even be called...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fine, there is no need for doing so.

unwindset.parse_unwind(options.get_option("unwind"));
unwindset.parse_unwindset(
options.get_list_option("unwindset"), goto_model, ui_message_handler);
setup_symex(symex, ns, ui_message_handler);
Copy link
Member

Choose a reason for hiding this comment

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

see above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fine, there is no need for doing so.

@tautschnig tautschnig merged commit b0156a3 into diffblue:develop Sep 15, 2025
41 checks passed
@tautschnig tautschnig deleted the language_mode-protected branch September 15, 2025 12:08
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