Skip to content

Conversation

@smowton
Copy link
Contributor

@smowton smowton commented Apr 21, 2017

This adds a wrapper for the needed-methods and needed-classes objects used to accumulate dependencies during lazy loading, such that the first time a class is noted needed, its static initializer if any is also queued for elaboration.

The test case is borrowed from @reuk.

@tautschnig
Copy link
Collaborator

@smowton could you please take a look at the overly long lines reported?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit picking: STL headers first, blank line, local include

Copy link
Collaborator

Choose a reason for hiding this comment

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

Function comment headers missing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Single-character indent - no indent needed for public/private/protected

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is one of those a vector while the other is a set?

@smowton smowton force-pushed the fix/lazy_methods_and_clinit branch 3 times, most recently from 94832db to 5bbec11 Compare April 21, 2017 11:56
@smowton
Copy link
Contributor Author

smowton commented Apr 21, 2017

Amended and added comments to address your question about vector vs. set.

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

See one more comment for a suggested improvement, but otherwise looks good to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for the clarification; I'd suggest to use std::unordered_set<irep_idt, irep_id_hash> &needed_classes as a minor improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd want to make change a few others too, so I'll do that in a subsequent PR

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for taking a const ref to a temporary here vs. the following?

const irep_idt clinit_name(id2string(class_symbol_name)+".<clinit>:()V");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, fixed

…blue#846

This adds a wrapper for the needed-methods and needed-classes objects used
to accumulate dependencies during lazy loading, such that the first time a
class is noted needed, its static initializer if any is also queued for
elaboration.

The test case is borrowed from @reuk.
@smowton smowton force-pushed the fix/lazy_methods_and_clinit branch from 5bbec11 to 286fcc5 Compare April 21, 2017 13:02
@kroening kroening merged commit 10015b1 into diffblue:master Apr 27, 2017
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.

4 participants