Skip to content

Refactor/virtual register languages #6220

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Aug 3, 2021
Merged

Refactor/virtual register languages #6220

merged 5 commits into from
Aug 3, 2021

Conversation

martin-cs
Copy link
Collaborator

This makes a number of refactorings to the registration of languages. They are in increasing order of controversy and I would suggest reviewing them commit by commit.

This connects to #6219 as it means that more tools are usable with the other front-ends.

@martin-cs
Copy link
Collaborator Author

@danielsn for RMC / json_symtab related changes.

@martin-cs martin-cs force-pushed the refactor/virtual_register_languages branch from a9394cf to 27c4b98 Compare July 9, 2021 13:25
@martin-cs
Copy link
Collaborator Author

One issue with this PR is that we will now have four files that are basically identical. I had wanted to make this into the default implementation of register_languages but that makes util depend on the language front-ends which is not what we want. Solutions to this that reduce this duplication would be most appreciated.

@martin-cs martin-cs force-pushed the refactor/virtual_register_languages branch from 27c4b98 to c37f04c Compare July 9, 2021 13:53
@kroening
Copy link
Member

kroening commented Jul 9, 2021

Please see my comment on #6219 -- there should not be further extension of CBMC to support non-C languages.

@martin-cs martin-cs force-pushed the refactor/virtual_register_languages branch from c37f04c to 576015e Compare July 9, 2021 17:16
@martin-cs
Copy link
Collaborator Author

OK. I have removed the last commit which does that. Is the symtab2gb to cbmc route OK?

@codecov
Copy link

codecov bot commented Jul 9, 2021 •
edited
Loading

Codecov Report

Merging #6220 (021116b) into develop (4844d22) will increase coverage by 0.01%.
The diff coverage is 96.20%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6220      +/-   ##
===========================================
+ Coverage    76.17%   76.19%   +0.01%     
===========================================
  Files         1484     1486       +2     
  Lines       162317   162389      +72     
===========================================
+ Hits        123646   123729      +83     
+ Misses       38671    38660      -11
Impacted Files Coverage Δ
jbmc/src/jdiff/jdiff_parse_options.h 100.00% <ø> (ø)
src/ansi-c/expr2c_class.h 100.00% <ø> (ø)
src/goto-analyzer/goto_analyzer_parse_options.cpp 71.98% <ø> (-0.34%) ⬇️
src/goto-cc/goto_cc_languages.cpp 100.00% <ø> (ø)
src/goto-diff/goto_diff_parse_options.h 100.00% <ø> (ø)
src/goto-harness/goto_harness_parse_options.cpp 68.37% <ø> (ø)
...rc/goto-instrument/goto_instrument_parse_options.h 100.00% <ø> (ø)
...rc/memory-analyzer/memory_analyzer_parse_options.h 100.00% <ø> (ø)
src/symtab2gb/symtab2gb_parse_options.h 100.00% <ø> (ø)
src/util/parse_options.h 50.00% <0.00%> (-50.00%) ⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6f6970...021116b. Read the comment docs.

@martin-cs martin-cs force-pushed the refactor/virtual_register_languages branch from 576015e to 251dabb Compare July 11, 2021 10:47
@martin-cs martin-cs force-pushed the refactor/virtual_register_languages branch 2 times, most recently from 731daaa to 7335e74 Compare July 14, 2021 11:20
@martin-cs
Copy link
Collaborator Author

@tautschnig @peterschrammel @chrisr-diffblue this PR is now less controversial and is mostly a tidy up and refactor. Any chance of a review when you have a moment?

@martin-cs
Copy link
Collaborator Author

@TGWDB As you are looking at #6219 it would be good to have your review on this.

TGWDB
TGWDB approved these changes Jul 14, 2021
View reviewed changes
Copy link
Contributor

@TGWDB TGWDB left a comment

Choose a reason for hiding this comment

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

This looks ok to me. I don't have significant comment on the question of how to reduce duplicated code in different places.

@martin-cs martin-cs force-pushed the refactor/virtual_register_languages branch 2 times, most recently from 1fb9ea8 to cfe2678 Compare July 16, 2021 16:33
NlightNFotis
NlightNFotis approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

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

Solid refactoring 👍🏻

@martin-cs
Copy link
Collaborator Author

@chrisr-diffblue while you are reviewing things, any chance of a look at this?

chrisr-diffblue
chrisr-diffblue approved these changes Jul 20, 2021
View reviewed changes
src/memory-analyzer/memory_analyzer_parse_options.cpp
void memory_analyzer_parse_optionst::register_languages()
{
// For now only C is supported due to the additional challenges of
// mapping source code to debugging symbols in other languages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth a reference to a GitHub issue here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't know there was one! I'll have a look...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not immediately finding one; if you had one in mind can you post a link?

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies for the chasing of the geese that are wild (positively livid...), I sort of half-assumed this was referring to the tickets discussing possible refactorings around languaget and modes - but I realise now I mis-read this comment.

@martin-cs martin-cs force-pushed the refactor/virtual_register_languages branch 3 times, most recently from 5001c9f to 603ca7a Compare July 27, 2021 09:57
@martin-cs
Copy link
Collaborator Author

@tautschnig @peterschrammel @romainbrenguier you are code-owners for this, please can you review when you have time? I have removed the controversial commit.

@martin-cs martin-cs force-pushed the refactor/virtual_register_languages branch from 603ca7a to 4d574dc Compare July 27, 2021 16:24
tautschnig
tautschnig approved these changes Aug 3, 2021
View reviewed changes
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

Thank you for this cleanup!

@martin-cs martin-cs force-pushed the refactor/virtual_register_languages branch from 4d574dc to 021116b Compare August 3, 2021 20:28
@martin-cs
Copy link
Collaborator Author

@peterschrammel or @romainbrenguier I need your sign-off for this one.

peterschrammel
peterschrammel approved these changes Aug 3, 2021
View reviewed changes
Copy link
Member

@peterschrammel peterschrammel left a comment

Choose a reason for hiding this comment

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

Lgtm

@martin-cs
Copy link
Collaborator Author

Thanks for the fast review. I will merge when CI passes.

@martin-cs martin-cs merged commit 06afe80 into diffblue:develop Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisr-diffblue chrisr-diffblue chrisr-diffblue approved these changes

@tautschnig tautschnig tautschnig approved these changes

@NlightNFotis NlightNFotis NlightNFotis approved these changes

@peterschrammel peterschrammel peterschrammel approved these changes

@TGWDB TGWDB TGWDB approved these changes

@kroening kroening Awaiting requested review from kroening kroening is a code owner

@romainbrenguier romainbrenguier Awaiting requested review from romainbrenguier

Assignees

@peterschrammel peterschrammel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@martin-cs @kroening @tautschnig @NlightNFotis @peterschrammel @chrisr-diffblue @TGWDB