Skip to content

Conversation

mgudemann
Copy link
Contributor

This is a first step for Java8 lambda support based on invokedynamic. This parses the BootstrapMethods attribute in a class files, verifies that at most one exists and stores the information about referred lambda functions.

This is based on some hypotheses:

  • lambda function invokedynamic will always use LambdaFactory metafactory or altmetafactory for CallSite generation
  • compiled lambda functions start with lambda$ name, followed by new$, static$ or $FunctionName$` and running number

If the above hypotheses are not fulfilled, the entry in lambda_method_handles_map is set to UNKNOWN, no explicit invariants are used here beyond class file format restrictions.

@mgudemann mgudemann self-assigned this Feb 7, 2018
@mgudemann mgudemann force-pushed the feature/parse_bootstrapmethods_attribute branch 2 times, most recently from 014540e to 257e5e2 Compare February 7, 2018 09:18
u2 arg_index2 = read_u2();
u2 arg_index3 = read_u2();

// skip rest
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify in the comment what we lose by ditching this stuff?

real_handle.method_type = pool_entry(arg3.ref1).s;
parsed_class.lambda_method_handle_map[parsed_class.name].push_back(
real_handle);
status()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is surely debug-level output

}
else
{
// skip arguments here
Copy link
Contributor

Choose a reason for hiding this comment

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

Again clarify what we're throwing away

lambda_method_handlet empty_handle;
parsed_class.lambda_method_handle_map[parsed_class.name].push_back(
empty_handle);
// skip arguments here
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

// where $POSTFIX is $FUN for a function name in which the lambda is define
// "static" when it is a static member of the class
// "new" when it is a class variable, instantiated in <init>
if(has_prefix(id2string(pool_entry(nameandtype_entry.ref1).s), "lambda$"))
Copy link
Contributor

Choose a reason for hiding this comment

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

else if.

Also remove linebreaks between end of if and start of else if to clarify that they belong together

handle.handle_type = method_handle_typet::LAMBDA_METHOD_HANDLE;
}

else if(
Copy link
Contributor

Choose a reason for hiding this comment

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

else return false ? This can clearly fail; the caller should do something appropriate in that case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the handle parameter is initialized with UNKNOWN so if nothing matches this simply stays. Overall probably makes more sense to return an optional method handle, though.

@thk123 thk123 requested a review from majakusber February 8, 2018 10:17
Copy link

@majakusber majakusber left a comment

Choose a reason for hiding this comment

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

It looks good to me, just two minor comments. I was also thinking if a unit test would make sense, to test that the parsed info has the expected form in an example. The regression test here only shows that the attribute is processed.

"only one BootstrapMethods argument is allowed in a class file");

// mark as read in parsed class
parsed_class.read_attribute_bootstrapmethods = true;

Choose a reason for hiding this comment

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

Why do we need to mark it read? According to java specification, there cannot be more than one BootstrapMethods attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be worth sanity checking, that we only have one copy of this attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@svorenova this is to validate that this hypothesis holds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@svorenova the current problem is that this is done in the class loader which is not accessible at the moment.
I agree that a unit test would be good (I had started one but then realized the above) but suggest deferring this to once we now how this information will finally be used and make it accessible in some form then.

typedef java_bytecode_parse_treet::classt::method_handle_typet
method_handle_typet;
typedef java_bytecode_parse_treet::classt::lambda_method_handlest
lambda_method_handlest;

Choose a reason for hiding this comment

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

This is renamed in the next line, so should be removed.

u2 num_bootstrap_methods = read_u2();
for(size_t i = 0; i < num_bootstrap_methods; i++)
{
u2 bootstrap_methodhandle_ref = read_u2();
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the programming standards, basic words should be separated by _ and abbreviations should be avoided. Therefore bootstrap_methodhandle_ref should be bootstrap_method_handle_reference.

Choose a reason for hiding this comment

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

I think the names were taken from the Java spec: https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.7.23
So the question is whether to stay consistent with the spec or with general standards. In other parts of our code for parsing bytecode that I've seen, the names follow the Java spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add this link / a reference to the relevant section of the specification in a comment somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that names corresponding to specific concepts should match the java spec rather than a strict interpretation of our own naming convention.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok with the original java standards naming.


// read method handle pointed to from constant pool entry at index, supports
// reading method handles for bootstrap methods and lambda methods
void java_bytecode_parsert::parse_methodhandle(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should parse_methodhandle be parse_method_handle?

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 don't mind, but just as the cases above, methodhandle is used like this in the JVM class file format spec

// reading method handles for bootstrap methods and lambda methods
void java_bytecode_parsert::parse_methodhandle(
u2 index,
u2 num_bootstrap_arguments,
Copy link
Contributor

Choose a reason for hiding this comment

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

"num" is an abbreviation which according to the programming standards should be avoided. So, non abbreviated this would be number_bootstrap_arguments, but I think bootstrap_arguments_count might read better.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is ok, as num_bootstrap_arguments is in the java specification.

"reference kind of Methodhandle must be in the range of 1 to 9");

const auto &class_entry = pool_entry(ref_entry.ref1);
const auto &nameandtype_entry = pool_entry(ref_entry.ref2);
Copy link
Contributor

Choose a reason for hiding this comment

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

nameandtype_entry should be name_and_type_entry

Copy link
Contributor

Choose a reason for hiding this comment

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

This is ok as it matches the java specification.

{
// skip arguments here
for(size_t i = 0; i < num_bootstrap_arguments; i++)
read_u2();
Copy link
Contributor

Choose a reason for hiding this comment

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

Although the coding standards states that "Single line blocks without { } are allowed", my preference is that { } is still used for single line blocks. I find that adding the { }, reduces the chance of mistakes when expanding the block to multiple lines later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it may be better to use a call to skip_bytes (or a new skip_u2s) method rather than writing an additional loop each time you want to skip things.

/// for a recognized bootstrap method, for a lambda function or unknown
void java_bytecode_parsert::parse_methodhandle(
const pool_entryt &entry,
lambda_method_handlet &handle)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could handle be the return value instead of an output argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will change

UNKNOWN_HANDLE
};

struct lambda_method_handlet
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a new class instead of a struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't the main difference that struct members are public by default while class members are private ? But yes, no problem to make this a class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, C++ doesn't make a large differentiation between the struct and class keywords. I am asking for a class to be used with each of the fields accessed through getter and setter methods instead of publicly accessible fields. The production of the debug text on lines 1480-1485, could then be defined as a method instead of inline at the point of building the class.
I know this is a relatively small example of a class to build with getters/setters which feel like boiler plate code. However I think it gives a superior foundation to build on, leading to more compartmentalised code in the end result, instead of large individual files.

const pool_entryt &arg3 = pool_entry(arg_index3);

if(!(arg1.tag == CONSTANT_MethodType &&
arg2.tag == CONSTANT_MethodHandle &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we check the tag of arg2 and throw the value away instead of storing for later use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just to validate that the format is what we expect a lambda entry to look like

<< id2string(real_handle.lambda_method_name) << " in class \""
<< parsed_class.name << "\""
<< "\n interface type is "
<< id2string(real_handle.interface_type = pool_entry(arg1.ref1).s)
Copy link
Contributor

@thomasspriggs thomasspriggs Feb 8, 2018

Choose a reason for hiding this comment

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

Why are we performing an assignment in the middle of a status output? Wasn't real_handle.interface_type already assigned to pool_entry(arg1.ref1).s back on line 1476?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, definitely a copy&paste bug

<< "\n interface type is "
<< id2string(real_handle.interface_type = pool_entry(arg1.ref1).s)
<< "\n method type is "
<< id2string(real_handle.interface_type = pool_entry(arg3.ref1).s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't real_handle.interface_type = pool_entry(arg3.ref1).s be real_handle.method_type?

{
u2 bootstrap_methodhandle_ref = read_u2();
const pool_entryt &entry = pool_entry(bootstrap_methodhandle_ref);
u2 num_bootstrap_arguments = read_u2();
Copy link
Contributor

Choose a reason for hiding this comment

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

"num" is an abbreviation, so according to the standards this should be number_bootstrap_arguments, but I think bootstrap_arguments_count would read better.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the naming from the java standards, so it is ok.

{
lambda_method_handlet empty_handle;
parsed_class.lambda_method_handle_map[parsed_class.name].push_back(
empty_handle);
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are cases where we may need to deal with empty / missing handles, then perhaps the type of the lambda_method_handle_map should be std::map<irep_idt, optionalt<lambda_method_handlest> > instead of std::map<irep_idt, lambda_method_handlest>

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps it isn't worth adding them to the map at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

best would probably to have a mapping from pairs of class name and entry number to the handle, then a non-existing (or non-parsable) lambda method handle would simply not exist in the map

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.

Only reviewed first commit so far.

}
else if(attribute_name == "BootstrapMethods")
{
INVARIANT(
Copy link
Contributor

Choose a reason for hiding this comment

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

Preferably comment with relevant spec that says this

Copy link
Contributor

Choose a reason for hiding this comment

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

Java 8 class file spec §4.4.10

+ id2string(pool_entry(nameandtype_entry.ref2).s);

if(
method_name ==
Copy link
Contributor

Choose a reason for hiding this comment

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

Eek! Perhaps we need some kind of interface that specifies whether for a given method name whether it can handle it? Could you add to the PR a summary of how this step is structured:

stores the information about referred lambda functions.

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.

mid way through reviewing but have a meeting so thoughts so far (I strongly agree with @svorenova that unit tests would allow much more precise evaulation of whether the parse tree looks like what we want it to)

}
else
{
real_handle.interface_type = pool_entry(arg1.ref1).s;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs some kind of comment to explain why arg1.ref1 is the interface type!

@@ -0,0 +1,3 @@
interface CustomLambda<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not both? 😛

for(size_t i = 0; i < num_bootstrap_methods; i++)
{
u2 bootstrap_methodhandle_ref = read_u2();
const pool_entryt &entry = pool_entry(bootstrap_methodhandle_ref);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding invariant that this entry is a CONSTANT_MethodHandle_info?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is actually handled in parse_methodhandle

}
else if(attribute_name == "BootstrapMethods")
{
INVARIANT(
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably pull out rBootStrapMethods_attribute method

}
else if(attribute_name == "BootstrapMethods")
{
INVARIANT(
Copy link
Contributor

Choose a reason for hiding this comment

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

Java 8 class file spec §4.4.10

status() << "INFO: parse BootstrapMethod handle "
<< num_bootstrap_arguments << " #args"
<< eom;
parse_methodhandle(entry, handle);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have this return the handle?

{
u2 bootstrap_methodhandle_ref = read_u2();
const pool_entryt &entry = pool_entry(bootstrap_methodhandle_ref);
u2 num_bootstrap_arguments = read_u2();
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than reading the arguments below - I'd be tempted to read them all now into an appropriate vector so there's no chance of missing a byte (e.g. if we add more handle_types)

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.

Got to the bit where we start comparing method_name. Up until the end was looking good, but I think the last view decoding steps are too dense to really follow along even with the spec open next to it.

I also think that we need to design exactly how these different supported times are structured, potentially outside the scope of this PR, which would just read the information into some kind of primitive structure.

const auto &ref_entry = pool_entry(entry.ref2);
INVARIANT(
(entry.ref1 > 0 && entry.ref1 < 10),
"reference kind of Methodhandle must be in the range of 1 to 9");
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add reference to §5.4.3.5

INVARIANT(
entry.tag == CONSTANT_MethodHandle,
"constant pool entry must be a MethodHandle");
const auto &ref_entry = pool_entry(entry.ref2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not new code - but I'm a bit confused by this, the spec seems to suggest that a pool entry is a u1 tag followed by an array of u1s dependent on the tag. But our pool_entryt is two u2s, a string, a u8 and an expression. Perhaps should raise a technical debt to have a different class for each pool entry type or at least have the pool_entryt mirror how it looks in the bytecode and have methods that look like:

u1 get_method_handle_reference_kind(const pool_entryt &entry) {
  INVARIANT(entry.get_tag()==CONSTANT_MethodHandle);
  return entry.data[0];
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think as a minimum you should introduce that latter method for the new pool_entryt so that is at least obvious that entry.ref2 is the reference_index

(entry.ref1 > 0 && entry.ref1 < 10),
"reference kind of Methodhandle must be in the range of 1 to 9");

const auto &class_entry = pool_entry(ref_entry.ref1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit is kind of complex to follow, suggest mirroring the spec to first create an enum for the different values of the reference_kind:

REF_getField
REF_getStatic
REF_putField
REF_putStatic
REF_invokeVirtual
REF_invokeStatic
REF_invokeSpecial
REF_newInvokeSpecial
REF_invokeInterface

Then switch on these in the same way, breaking into rFieldref_info

I'm pausing here as it has got hard to follow - but the special casing of dealing with which factories we actually support should be abstracted far away from the parse tree IMO (in fact potentially not even part of this PR?)

@thk123
Copy link
Contributor

thk123 commented Feb 9, 2018

For unit tests, can you not do something like:

SCENARIO(
"rclass_attribute",
"[core][java_bytecode][java_bytecode_parse_generics]")
{
  ui_message_handlert message_handler;
  java_bytecode_parse_treet parse_tree;
  java_bytecode_parse("/home/tkiley/workspace/cbmc/regression/cbmc-java/lambda1/Lambdatest.class", parse_tree, message_handler);

  REQUIRE(parse_tree.loading_successful);


  REQUIRE(parse_tree.parsed_class.lambda_method_handle_map.size() > 0);
  // etc.

}

(note this currently fails on the Lambdatest.class - don't know if this is correct (i.e. should there be something in the lambda_method_handle_map) but I think the principle is OK.

I had originally hoped you could test the individual read methods, but of couse you don't know how far through the bytecode you need to go to start reading the class attributes for example. Of course, really nice would be to rather than using real class files, we write subsections of class files that explore all the ways the bytecode for that structure can be. But that is for perhaps another day.


const auto &class_entry = pool_entry(ref_entry.ref1);
const auto &nameandtype_entry = pool_entry(ref_entry.ref2);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be a double check here that name_and_type_entry.tag is set to CONSTANT_NameAndType

Copy link
Contributor

@thk123 thk123 Feb 15, 2018

Choose a reason for hiding this comment

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

I believe this comment is addressed by mgudemann#1

<< "\n interface type is "
<< id2string(pool_entry(interface_type_argument.ref1).s)
<< "\n method type is "
<< id2string(pool_entry(method_type_argument.ref1).s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you refactor the generation of this debugging text into a std::string pretty() method of lambda_method_handlet class? I think it would be useful to be able to generate this later whilst debugging lambda functionality, instead of just at the point where the lambda function is parsed.

@thk123
Copy link
Contributor

thk123 commented Feb 14, 2018

@mgudemann I have created mgudemann#1 to address the comments I made, if you like them, I guess you can merge the PR with a rebase and they will appear here?

@thk123
Copy link
Contributor

thk123 commented Feb 15, 2018

As discovered when writing unit tests for this (btw - a strong argument for unit tests 😛 ) - the classt::swap method needs updating as part of this PR

@thk123
Copy link
Contributor

thk123 commented Feb 15, 2018

So I see by removing the special handling in the parse_method_handle I deactivated the code that actually turns the lambdas into the actual function it corresponds to.

That is, we first read a method handle that corresponds to (probably) a lambdafactory method. We then assume the parameters meet a certain criteria that allows us to get a second methodhandle that is referring to the implementation of the lambda.

I had hoped to avoid cluttering the bytecode parsing with the specifics of how lambads work, but since we loose the constants pool we need to do something.

I suggest rather than reusing the lambda_method_handle for both, I introduce another method_handle_infot for the general case, then we have a separate class that takes these, and the constants pool and converts them into a lambda_method_handle to go into the table.

I'll put the first part in an update to mgudemann#1 and carry on with the unit tests. If you'd be able to pull out the bit that starts with // try parsing bootstrap method handle into a separate file then we'll have a good base to build on.

@thk123
Copy link
Contributor

thk123 commented Feb 15, 2018

I've updated mgudemann#1 with the suggested change while leaving the special case handling in java_bytecode_parser.cpp - will now do what I should have been doing and actually write the unit tests!

@mgudemann mgudemann force-pushed the feature/parse_bootstrapmethods_attribute branch from ddbf4f5 to 44b9f07 Compare February 19, 2018 14:01
@mgudemann
Copy link
Contributor Author

@thk123 is there a special reason why you'd like to have the parsing refactored into a separate file? I currently refactored it into its own method in java_bytecode_parsert that is called from rclass_attribute

@thk123
Copy link
Contributor

thk123 commented Feb 19, 2018

@mgudemann Only that it feels like it might expand if/when we support other methods called by invoke_dynamic (and the file is already quite large). I think a similar argument could be made for the structured pool stuff.

@mgudemann
Copy link
Contributor Author

@thk123 ok then I'll split the bootstrapmethods parsing and the structured constant pool stuff into an extra file within the java_bytecode folder. How about a java_lambda_functions file?

@thk123
Copy link
Contributor

thk123 commented Feb 20, 2018

@mgudemann I would put the constants pool stuff into a structured_constant_pool_entries.cpp as I have a vague hope that we can transition to that for all the constants to give us a bit more structure when converting constants.

@mgudemann mgudemann force-pushed the feature/parse_bootstrapmethods_attribute branch from e097cb1 to bf53c54 Compare February 21, 2018 08:25
@mgudemann mgudemann force-pushed the feature/parse_bootstrapmethods_attribute branch from c8bc9cf to 5daf6e1 Compare February 27, 2018 15:52
Copy link

@majakusber majakusber left a comment

Choose a reason for hiding this comment

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

Looks good, love the regression/unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

If parse_method_handle returns an empty optionalt then the code will reach this line and crash due to a failed assertion on .has_value() inside the implementation of the -> operator in optionalt. If we mean to halt execution in this case then we should have a CHECK_RETURN() on the return from parse_method_handle. Assuming of course that we don't have a more elegant way of handling this without halting execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is now handled ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I first read this I thought - "Why are we using .swap rather than just assignment with =?" Then I realised that this was an optimisation to avoid having two copies of the data in the vector in memory at once. The C++11 way to do this would be lambda_method_handle.u2_values = std::move(u2_values); I think this makes the intent clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

A minor optimisation would be to use u2_valuest u2_values(num_bootstrap_arguments);, so that the vector is created with the correct amount of elements reserved instead of resized as we add them.

@mgudemann mgudemann force-pushed the feature/parse_bootstrapmethods_attribute branch 2 times, most recently from a347a2e to 552c7ae Compare February 27, 2018 16:53
@mgudemann mgudemann force-pushed the feature/parse_bootstrapmethods_attribute branch from 552c7ae to 76d4014 Compare February 27, 2018 17:32
Copy link
Contributor

@thomasspriggs thomasspriggs left a comment

Choose a reason for hiding this comment

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

Ok, can we get this merged now?

@thk123 thk123 merged commit 5cdfa94 into diffblue:develop Feb 28, 2018
@mgudemann mgudemann deleted the feature/parse_bootstrapmethods_attribute branch February 28, 2018 10:07
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