Passing base::Value by unique_ptr instead of raw pointers

[Václav Brožek]

Hi all,

This is just a heads-up that I have been and will be for some foreseeable future replacing passing base::Value ownership by raw pointer with passing that ownership by unique_ptr. The goal is to enable deleting the old Value API with raw pointers (in favour of the already available unique_ptr equivalents), as well as to make the migration to value-based Value API (happening in https://crbug.com/646113) easier. One particular goal is also to expose and fix leaks and other errors and fix issues like https://crbug.com/697817. The latter bug is also where I track my progress with the change to avoid raw pointers.

If you see one of my CLs and have any concerns, please do not hesitate to reach out to me.

I expect to be using TBR on my patches for trivial changes and will of course follow our guidelines for that.

Cheers,
Vaclav

[Brett Wilson]

Hi Václav,

Since we're moving to value semantics, might it be less work in the long run to skip unique_ptr and change to move semantics?

Most of these changes are probably using the dictionary and list value types. I think the APIs for these should have versions that take const refs and moves. Otherwise we will have to replace all of the unique_ptr calls later with this.

Brett

[Václav Brožek]

Hi Brett,

TL;DR -- it seems to me like the mid-step with unique_ptr, while adding a little overhead, might allow us to paralellise the migration to value semantics more.

More details:

I though about skipping the unique_ptr, but that would mean blocking the change until the new API is ready.

There still seems to be value(*) in doing the mid-step with unique_ptr in that it makes the intermediate steps smaller. Consider the following patter which I saw frequently in the code:

DictionaryValue* value = new DictionaryValue(...);

...

FunctionTakingOwnership(value);

...

value->Set(...);

To convert this directly to the value semantics API, one needs to both fix the all the lines touching |value| and also fix the order so that the third line comes before the second one. So the one-step change introduces significant churn.

Compared to that, converting to unique_ptr first still creates most of that churn (some changes on lines touching |value| and the change in the order), but because of the current co-existence of raw-pointer and unique_ptr APIs, these conversions can be better separated into smaller chunks. Once converted to unique_ptr, the code would look something like:

auto value = MakeUnique<DictionaryValue>();

...

value->Set(...);

FunctionTakingOwnership(std::move(value));

Changing that to value semantics API is a fairly minimal change: only changes the |value| declaration and all "->" into ".". Also, bugs in the code caused by the the raw pointer ownership being unclear are unlikely to still be present during the conversion to value semantics, making that conversion smoother and smaller in scope.

So while you and Jan are progressing towards the value semantics, I thought I would prepare the code in advance for a smoother transition, instead of the whole project being a long sequence of strongly dependent tasks. (I also still hope to root out the potentially multiple causes of https://crbug.com/697817 ASAP.)

Does that make sense?

Cheers,
Václav

(*) Apologies for using "value" with at least 3 different meanings in this e-mail!

[Brett Wilson]

If you really want to do it this way, I'm fine with that, the unique_ptr thing is a strict improvement over what we have now.

Brett