Skip to content

Reduce compiletest code for easier decoupling#694

Merged
adpaco-aws merged 4 commits intomodel-checking:mainfrom
adpaco-aws:shrink-compiletest
Dec 22, 2021
Merged

Reduce compiletest code for easier decoupling#694
adpaco-aws merged 4 commits intomodel-checking:mainfrom
adpaco-aws:shrink-compiletest

Conversation

@adpaco-aws
Copy link
Copy Markdown
Contributor

@adpaco-aws adpaco-aws commented Dec 14, 2021

Description of changes:

Refactors the compiletest tool to remove most code not used in this project while still keeping its major features (suite testing, caching, concurrency). This allows us to decouple from the compiler anytime in the future.

For comparison, this is the cloc output on src/tools/compiletest (branch main):

      16 text files.
      16 unique files.                              
       0 files ignored.

github.com/AlDanial/cloc v 1.82  T=0.03 s (515.1 files/s, 271922.2 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Rust                            15           1041            797           6583
TOML                             1              3              0             23
-------------------------------------------------------------------------------
SUM:                            16           1044            797           6606
-------------------------------------------------------------------------------

The output including the changes in this PR:

       9 text files.
       9 unique files.                              
       0 files ignored.

github.com/AlDanial/cloc v 1.82  T=0.01 s (695.0 files/s, 136687.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Rust                             8            205            217           1322
TOML                             1              3              0             23
-------------------------------------------------------------------------------
SUM:                             9            208            217           1345
-------------------------------------------------------------------------------

So about 78% (1345/6606) of the code has been remove.

Note that option parsing has been changed to make all arguments optional so the tool still works when called from x.py. Most code there will be removed at the time of decoupling.

Noticed that #289 is not an issue anymore (probably due to other changes in rmc.py). Removed the code to disable related checks and reference to the issue.

Resolved issues:

Resolves #289

Call-outs:

Testing:

  • How is this change tested? Running existing regression.

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

@adpaco-aws adpaco-aws requested a review from a team as a code owner December 14, 2021 16:32
Copy link
Copy Markdown
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

That's awesome! I would prefer if we copy the heavily modified compiletest to a different location and keep the current one as is. I'm worried that this may make the next upstream synchronization much harder.

@adpaco-aws adpaco-aws force-pushed the shrink-compiletest branch 2 times, most recently from 6de4bb6 to 189966e Compare December 14, 2021 21:53
@adpaco-aws
Copy link
Copy Markdown
Contributor Author

Copied the original proposal into src/tools/rmctest and replaced the path to compiletest in bootstrap. Unfortunately, the copyright check is failing to pass - do we add it or override?

@adpaco-aws
Copy link
Copy Markdown
Contributor Author

Went back to the original proposal. Added "modifications" copyright notices for each file.

Copy link
Copy Markdown
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks

@adpaco-aws adpaco-aws merged commit 2fb5bd1 into model-checking:main Dec 22, 2021
@adpaco-aws adpaco-aws deleted the shrink-compiletest branch December 22, 2021 21:30
tedinski pushed a commit to tedinski/rmc that referenced this pull request Apr 26, 2022
* Reduce compiletest code for easier decoupling

* Add modifications copyright

* Remove unused function `get_lib_name`
tedinski pushed a commit that referenced this pull request Apr 27, 2022
* Reduce compiletest code for easier decoupling

* Add modifications copyright

* Remove unused function `get_lib_name`
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.

Default flags in cargo-rmc cause CBMC inviariant violation

2 participants