Skip to content

Split parse_list_types into two functions #3293

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

Merged

Conversation

JohnDumbell
Copy link
Contributor

@JohnDumbell JohnDumbell commented Nov 8, 2018

In some situations you might want to know what the bytecode type is and then manually perform some operation afterwards, in this case it's useful to have a method that returns the string representation of the bytecode type.

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

@JohnDumbell JohnDumbell force-pushed the jd/refactor/split_parse_list_types branch from 7a669a9 to ccd22e8 Compare November 8, 2018 11:47
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.

It'd be a great time to add a unit test for these methods

{
// Loop over the types in the given string, parsing each one in turn
// and adding to the type_list
std::vector<typet> type_list;
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️

const std::vector<std::string> type_strings = parse_raw_list_types(src, opening_bracket, closing_bracket); 
std::vector<typet> type_list;
std::transform(
  type_strings.begin(), 
  type_strings.end(), 
  std::back_inserter(type_list), 
  [&](const std::string &raw_type) {
    const typet new_type = java_type_from_string(raw_type, class_name_prefix);
    INVARIANT(new_type != nil_typet(), "Failed to parse type");
    return new_type;
});
return type_list;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this actually give any real benefit when you're simply looping through an entire list with no bells or whistles?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's exactly when you should use it, so the reader can tell that you are looping through an entire list with no bells and whistles! It wouldn't work if you had some whistles.

FYI the ⛏️ here means a non-blocking comment that you could fix if you're in the area but mostly there as something to try and do in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheers, was mostly just curious about the recommendation (wasn't meant as a refutal).

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: ccd22e8).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90702246
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
Copy link
Contributor

thk123 commented Nov 8, 2018

Joel bot is feeling a bit sensitive at the moment as previously merged CBMC PRs have broken TG - since this touches Java code can you wait until it comes back online? (hopefully shortly).

@tautschnig
Copy link
Collaborator

Please rebase to re-trigger the TG check.

In some situations you might want to know what the bytecode type is and then manually perform some operation afterwards, in this case it's useful to have a method that returns the string representation of the bytecode type.
@JohnDumbell JohnDumbell force-pushed the jd/refactor/split_parse_list_types branch from 0d8ceaf to 4898928 Compare November 9, 2018 15:51
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: 4898928).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/90880596

@tautschnig tautschnig merged commit 7835a19 into diffblue:develop Nov 9, 2018
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.

6 participants