Skip to content

Complexity graph #7081

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

Draft
wants to merge 36 commits into
base: develop
Choose a base branch
from
Draft

Complexity graph #7081

wants to merge 36 commits into from

Conversation

bquiring
Copy link

@bquiring bquiring commented Aug 22, 2022

  • 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.
  • 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).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@martin-cs
Copy link
Collaborator

Thanks for your contribution; it looks very exciting. We might need to do a little bit of work to make it ready for review. Please could you:

  • Write a short description of what this is supposed to do. I saw the addition to the documentation, that is great but it's also good to have some info in the PR about what your goal is, what the architecture is, why you have done things the way you have.
  • Please can you squash together commits that change the same thing. I realise it is hard when working on a new feature but if you can sort the changes into commits which each do one logical thing, it makes it much easier to review.
  • We will need some test case before this can be merged. Ideally end-to-end system tests (as in regressions but unit tests are also good if it establishes interfaces that others can use within the code).

@martin-cs
Copy link
Collaborator

Also, if possible, we shouldn't be adding commented out code. I realise it is handy during development and this is a draft PR but eventually it should become configurable or be removed.

@feliperodri feliperodri added new feature aws Bugs or features of importance to AWS CBMC users labels Aug 23, 2022
@@ -635,13 +637,53 @@ int cbmc_parse_optionst::doit()
verifier = util_make_unique<
all_properties_verifier_with_trace_storaget<multi_path_symex_checkert>>(
options, ui_message_handler, goto_model);
// TODO: this is the branch triggered by default
Copy link
Collaborator

Choose a reason for hiding this comment

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

What needs to be done here? Maybe file an issue on whatever it is instead of leaving this isn the code.

@@ -383,3 +383,8 @@ std::string solver_hardnesst::expr2string(const exprt expr)
ss << format(expr);
return ss.str();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could perhaps be its own PR.

@@ -129,6 +129,9 @@ struct solver_hardnesst : public clause_hardness_collectort
solver_hardnesst &operator=(const solver_hardnesst &) = delete;
solver_hardnesst &operator=(solver_hardnesst &&) = default;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be in it's own PR

@nwetzler nwetzler self-requested a review September 26, 2022 23:27
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 new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants