Skip to content

Cleanup RMC Flags#332

Merged
vecchiot-aws merged 14 commits intomodel-checking:main-153-2021-07-15from
vecchiot-aws:cleanup-rmc-flags
Jul 26, 2021
Merged

Cleanup RMC Flags#332
vecchiot-aws merged 14 commits intomodel-checking:main-153-2021-07-15from
vecchiot-aws:cleanup-rmc-flags

Conversation

@vecchiot-aws
Copy link
Copy Markdown
Contributor

@vecchiot-aws vecchiot-aws commented Jul 15, 2021

Description of changes:

This PR cleans up the flags in rmc and cargo-rmc by unifying them into a shared file, adding group organization, and descriptions for each flag.

Resolved issues:

Call-outs:

Testing:

  • How is this change tested? Existing regression suite.

  • Is this a refactor change? Yes.

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@vecchiot-aws vecchiot-aws changed the title Initial draft. Cleanup RMC Flags Jul 15, 2021
Copy link
Copy Markdown
Contributor

@nchong-at-aws nchong-at-aws left a comment

Choose a reason for hiding this comment

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

Good clean-up. Some suggested/requested changes on naming.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe proof_harness_flags?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider pointing to the cbmc-viewer docs for more info

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

retcode -> return code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove necessarily

Copy link
Copy Markdown
Contributor

@bdalrhm bdalrhm left a comment

Choose a reason for hiding this comment

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

Appreciate the help messages 🙂

Comment on lines 62 to 65
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
def add_flags(
parser,
exclude_flags=[],
exclude_groups=[]):
def add_flags(parser, exclude_flags=[], exclude_groups=[]):

Consider using a formatter (like autopep)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
parser = argparse.ArgumentParser(description="Verify a Rust crate")
parser = argparse.ArgumentParser(prog="cargo rmc", description="Verify a Rust crate")

cargo-rmc script is usually called as cargo rmc.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can we fail here instead of warn? Warnings are easy to overlook

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this isn't your change, but should this give a more helpful message?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The error is printed by dependencies_in_path, so we don't need to print anything here.

scripts/rmc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this specify in the last position?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It doesn't have to be in the last position; actually putting it first can avoid issues with flags that take multiple arguments. The only thing that really matters is --cbmc-args last, or else some RMC flags might be interpretted as CBMC flags.

@vecchiot-aws vecchiot-aws marked this pull request as ready for review July 22, 2021 19:50
@vecchiot-aws vecchiot-aws force-pushed the cleanup-rmc-flags branch 2 times, most recently from 623d9ed to 768573b Compare July 22, 2021 20:21
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Raise an error instead; "Error setting up command line parser..." + link to issue page for users to report bug.

@vecchiot-aws vecchiot-aws force-pushed the cleanup-rmc-flags branch 2 times, most recently from c4fa758 to 0f2064c Compare July 23, 2021 18:16
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we exclude these flags? Add a comment explaining why.

Comment on lines 9 to 14
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

alphabatize

Comment on lines 36 to 39
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sort

@vecchiot-aws vecchiot-aws merged commit df06679 into model-checking:main-153-2021-07-15 Jul 26, 2021
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.

6 participants