Skip to content

Add parsing of the names of inner java classes. #3599

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 4 commits into from
Dec 20, 2018
Merged

Add parsing of the names of inner java classes. #3599

merged 4 commits into from
Dec 20, 2018

Conversation

thomasspriggs
Copy link
Contributor

@thomasspriggs thomasspriggs commented Dec 19, 2018 •
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.
  • 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).
  • 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.

@thomasspriggs
Add parsing of the names of inner java classes
7e28299 
This is required for correctly printing the names of inner classes.
@thomasspriggs
Add unit test for inner class names
2dcbb00 
Added because it is good to test new features.
allredj
allredj reviewed Dec 19, 2018
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: 2dcbb00).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95277944
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 suggested changes Dec 19, 2018
View reviewed changes
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.

🚫 Please add a test for multiple inner classes inside the same outer class
I'd also like to see a test for starting at the outer class rather than the inner class.

jbmc/unit/java_bytecode/java_bytecode_parser/parse_inner_class.cpp Outdated

THEN("The inner class should have the inner name \"Inner\".")
{
REQUIRE(id2string(class_type->get_inner_name()) == "Inner");
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ Add a check on name as it isn't obvious to me why this isn't the name of the class

jbmc/unit/java_bytecode/java_bytecode_parser/parse_inner_class.cpp Outdated
WHEN("Parsing an inner class called \"Inner\".")
{
const symbol_tablet &symbol_table = load_java_class(
"Outer$Inner", "./java_bytecode/java_bytecode_parser");
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ What happens if you load from Outer rather than starting on the inner class?

thk123
thk123 reviewed Dec 20, 2018
View reviewed changes
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.

As discussed (?) please add a check for the inner class when starting from outer then LGTM

jbmc/unit/java_bytecode/java_bytecode_parser/parse_inner_class.cpp
THEN("The outer class does not have an outer class.")
{
REQUIRE(id2string(class_type->get_outer_class()).empty());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add tests for the inner class is loaded correctly when starting at the outer class.

@thomasspriggs
Copy link
Contributor Author

@thk123 Please re-review

allredj
allredj reviewed Dec 20, 2018
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: 28fbcce).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95410110

@thomasspriggs thomasspriggs merged commit deb2bf3 into diffblue:develop Dec 20, 2018
@thomasspriggs thomasspriggs deleted the tas/inner_class_names branch December 20, 2018 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allredj allredj allredj left review comments

@thk123 thk123 thk123 approved these changes

@peterschrammel peterschrammel peterschrammel 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

@smowton smowton Awaiting requested review from smowton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thomasspriggs @thk123 @peterschrammel @allredj