Skip to content

Java frontend: record source locations of read static variables #5144

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

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Oct 2, 2019

This enables trace-source-location reports not to vary depending on whether static initializer
calls were produced or not (at present the call would result in a statement at the getstatic
location, while the variable read itself would disappear; a 'putstatic' instruction by contrast
always generates an assignment and so is accounted for by a source location).

@smowton smowton force-pushed the smowton/feature/java-record-getstatic-source-locations branch from 35e1803 to e97ed5c Compare October 2, 2019 15:52
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: e97ed5c).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/130069395

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

LGTM

symbol_expr->source_location().get_java_bytecode_index() == "0" &&
symbol_expr->get_identifier() ==
"java::ClassReadingStaticField.x")
found = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ multi line condition so please add braces around the body


WHEN("Converting the method that reads said field")
{
const auto &method_symbol =
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ if the symbol isn't found, using lookup_ref makes the output on CI not very clear, better to use lookup and then REQUIRE(method_symbol != nullptr)

This enables trace-source-location reports not to vary depending on whether static initializer
calls were produced or not (at present the call would result in a statement at the getstatic
location, while the variable read itself would disappear; a 'putstatic' instruction by contrast
always generates an assignment and so is accounted for by a source location).
@smowton smowton force-pushed the smowton/feature/java-record-getstatic-source-locations branch from e97ed5c to 4c5ddc9 Compare October 5, 2019 10:55
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: 4c5ddc9).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/130528948

@codecov-io
Copy link

codecov-io commented Oct 5, 2019

Codecov Report

Merging #5144 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5144   +/-   ##
========================================
  Coverage    67.07%   67.07%           
========================================
  Files         1149     1149           
  Lines        94030    94030           
========================================
  Hits         63075    63075           
  Misses       30955    30955
Flag Coverage Δ
#cproversmt2 42.7% <ø> (ø) ⬆️
#regression 63.57% <100%> (ø) ⬆️
#unit 31.95% <100%> (ø) ⬆️
Impacted Files Coverage Δ
...java_bytecode/java_bytecode_convert_method_class.h 100% <ø> (ø) ⬆️
...src/java_bytecode/java_bytecode_convert_method.cpp 97.47% <100%> (ø) ⬆️

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 a33ac4e...4c5ddc9. Read the comment docs.

@smowton smowton merged commit aca06bd into diffblue:develop Oct 5, 2019
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.

5 participants