Skip to content

Clone reference array #3992

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 2 commits into from
Jan 31, 2019
Merged

Clone reference array #3992

merged 2 commits into from
Jan 31, 2019

Conversation

thk123
Copy link
Contributor

@thk123 thk123 commented Jan 30, 2019 •
edited
Loading

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

  • [n/a] 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).

  • My PR is restricted to a single feature or bugfix.

  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

  • Add descs for all combinations

@thk123
Copy link
Contributor Author

thk123 commented Jan 30, 2019 •
edited
Loading

I think c521ec9 is a better fix (the type of a reference array is an Object* not a void* (you can't put an int into a reference array, only an Integer which is a Object). However, this causes other problems - I'll investigate this a little once I've got the rest of the tests working and the TG tests re-enabled. Perhaps @smowton or others have some insight into why the original change would be a problem. It broke because somehow a dynamically sized array got down to symex.

@thk123 thk123 force-pushed the clone-reference-array branch from ee92c07 to e45ff42 Compare January 30, 2019 14:33
allredj
allredj reviewed Jan 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.

🚫
This PR failed Diffblue compatibility checks (cbmc commit: ee92c07).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/99131354
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

  • the cbmc commit has disappeared in the mean time (e.g. in a force-push)
  • the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

@thk123 thk123 force-pushed the clone-reference-array branch from e45ff42 to 2a5df74 Compare January 30, 2019 15:24
allredj
allredj reviewed Jan 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.

🚫
This PR failed Diffblue compatibility checks (cbmc commit: e45ff42).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/99137005
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

  • the cbmc commit has disappeared in the mean time (e.g. in a force-push)
  • the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

allredj
allredj reviewed Jan 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: 2a5df74).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/99145971

allredj
allredj reviewed Jan 30, 2019
View reviewed changes
jbmc/src/java_bytecode/java_types.cpp Outdated
@@ -209,7 +209,7 @@ typet java_type_from_char(char t)
case 'f': return java_float_type();
case 'd': return java_double_type();
case 'z': return java_boolean_type();
case 'a': return java_reference_type(void_typet());
case 'a': return java_lang_object_type();
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose you want to squash this commit with the last one?

Copy link
Contributor Author

@thk123 thk123 Jan 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes was hoping illicit some comments on the original plan - but will squash down and create a fresh PR fixing this would be better. (will wait for TG to pass first).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@thk123
Copy link
Contributor Author

thk123 commented Jan 30, 2019

TG bump passing.

Don't load nil types when lazy loading
9f1e528 
This can happen when processing the clone method which sets its return
type to be a void*
@thk123 thk123 force-pushed the clone-reference-array branch from 2a5df74 to f775c75 Compare January 30, 2019 17:29
LAJW
LAJW reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@LAJW LAJW left a comment

Choose a reason for hiding this comment

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

I'd block if this was test-gen, but just realized it's not D:

jbmc/src/java_bytecode/ci_lazy_methods_needed.cpp
if(element_type.id() == ID_pointer)
if(
element_type.id() == ID_pointer &&
element_type.subtype().id() != ID_empty)
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏ What about try_type_dynamic_cast<>?

allredj
allredj reviewed Jan 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: f775c75).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/99166627

allredj
allredj reviewed Jan 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: 5aae3a7).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/99174896

@thk123 thk123 force-pushed the clone-reference-array branch from 5aae3a7 to 4e919f3 Compare January 31, 2019 10:46
@thk123
Copy link
Contributor Author

thk123 commented Jan 31, 2019

@LAJW comments addressed (will do the try dynamic cast in a separate PR.

@thk123 thk123 force-pushed the clone-reference-array branch from 4e919f3 to 1e0d861 Compare January 31, 2019 11:15
@thk123 thk123 merged commit 8615e3b into diffblue:develop Jan 31, 2019
@thk123 thk123 deleted the clone-reference-array branch January 31, 2019 14:56
@thk123 thk123 mentioned this pull request Feb 4, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allredj allredj allredj approved these changes

@smowton smowton smowton approved these changes

@peterschrammel peterschrammel peterschrammel approved these changes

@LAJW LAJW LAJW approved these changes

@cesaro cesaro Awaiting requested review from cesaro

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

@NathanJPhillips NathanJPhillips Awaiting requested review from NathanJPhillips

@pkesseli pkesseli Awaiting requested review from pkesseli

Assignees

@thk123 thk123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thk123 @smowton @peterschrammel @LAJW @allredj