Skip to content

Cleanups in loading of jar files #4698

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
May 31, 2019
Merged

Cleanups in loading of jar files #4698

merged 5 commits into from
May 31, 2019

Conversation

NathanJPhillips
Copy link
Contributor

@NathanJPhillips NathanJPhillips commented May 23, 2019 •
edited
Loading

This includes adding a wrapper for the extract to file functionality to complement the extract to memory wrapper.

  • 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).
  • N/A My commit message includes data points confirming performance improvements (if claimed).
  • N/A 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.

@NathanJPhillips NathanJPhillips requested a review from smowton May 23, 2019 11:38
smowton
smowton reviewed May 23, 2019
View reviewed changes
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

I suspect you'll be asked to remove extract-to-file because it's unused

jbmc/src/java_bytecode/mz_zip_archive.cpp
return { buffer.cbegin(), null_char_it };
const auto id = static_cast<mz_uint>(index);
mz_uint name_size = mz_zip_reader_get_filename(m_state.get(), id, nullptr, 0);
if(name_size == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Braces round multi-line if

jbmc/src/java_bytecode/mz_zip_archive.cpp Outdated
{
const auto id = static_cast<mz_uint>(index);
mz_zip_archive_file_stat file_stat;
if(mz_zip_reader_file_stat(m_state.get(), id, &file_stat) != MZ_TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

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

file_stat is unused

allredj
allredj reviewed May 23, 2019
View reviewed changes
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

✔️
Passed Diffblue compatibility checks (cbmc commit: 31cfb2a).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/112922881

@NathanJPhillips
Copy link
Contributor Author

@smowton - happy to remove extract-to-file if you'd like - although there are other bits of functionality in JBMC that are only used by derivative projects. We can keep it for ourselves but it seemed like it might be useful to expose it for other potential users.

smowton
smowton approved these changes May 23, 2019
View reviewed changes
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

I'm happy with this as-is, just noting a common complaint re: committing untested / unused code :)

peterschrammel
peterschrammel reviewed May 23, 2019
View reviewed changes
jbmc/src/java_bytecode/mz_zip_archive.cpp
@@ -103,3 +103,16 @@ std::string mz_zip_archivet::extract(const size_t index)
}
throw std::runtime_error("Could not extract the file");
}

void mz_zip_archivet::extract_to_file(
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't seem to match the PR title.

tautschnig
tautschnig reviewed May 28, 2019
View reviewed changes
jbmc/src/java_bytecode/mz_zip_archive.cpp Outdated
const auto id = static_cast<mz_uint>(index);
mz_uint name_size = mz_zip_reader_get_filename(m_state.get(), id, nullptr, 0);
if(name_size == 0)
return {}; // Failure
Copy link
Collaborator

Choose a reason for hiding this comment

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

@NathanJPhillips clang-format would like to see one space removed.

@tautschnig
Copy link
Collaborator

@NathanJPhillips Please rebase now that #4729 is in to make CI happy.

allredj
allredj reviewed May 30, 2019
View reviewed changes
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

✔️
Passed Diffblue compatibility checks (cbmc commit: 6b5dfb1).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/113796715

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (develop@3f9e1b0). Click here to learn what that means.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #4698   +/-   ##
==========================================
  Coverage           ?   68.44%           
==========================================
  Files              ?     1255           
  Lines              ?   104099           
  Branches           ?        0           
==========================================
  Hits               ?    71250           
  Misses             ?    32849           
  Partials           ?        0
Impacted Files Coverage Δ
jbmc/src/java_bytecode/jar_file.h 100% <ø> (ø)
jbmc/src/java_bytecode/mz_zip_archive.h 100% <ø> (ø)
jbmc/src/java_bytecode/jar_file.cpp 80.95% <100%> (ø)
jbmc/src/java_bytecode/jar_pool.cpp 44.44% <100%> (ø)
jbmc/src/java_bytecode/mz_zip_archive.cpp 65.11% <68.75%> (ø)

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 3f9e1b0...72936d1. Read the comment docs.

allredj
allredj reviewed May 31, 2019
View reviewed changes
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

✔️
Passed Diffblue compatibility checks (cbmc commit: 72936d1).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/113895476

@tautschnig tautschnig merged commit 6bee122 into diffblue:develop May 31, 2019
@NathanJPhillips NathanJPhillips deleted the cleanup/jar-loading branch June 3, 2019 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tautschnig tautschnig tautschnig left review comments

@peterschrammel peterschrammel peterschrammel left review comments

@allredj allredj allredj left review comments

@smowton smowton smowton approved these changes

@cesaro cesaro Awaiting requested review from cesaro

@cristina-david cristina-david Awaiting requested review from cristina-david

@pkesseli pkesseli Awaiting requested review from pkesseli

@thk123 thk123 Awaiting requested review from thk123

Assignees

@NathanJPhillips NathanJPhillips

Labels
Needs rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@NathanJPhillips @tautschnig @codecov-io @smowton @peterschrammel @allredj