Replace SmartArrayPointer<T> with unique_ptr<T[]>

Replace SmartArrayPointer<T> with unique_ptr<T[]>

R=bmeurer@chromium.org
BUG=

Committed: https://crrev.com/37ba8f961bd4e18815f7c56683ed1ea4d2695b7d
Cr-Commit-Position: refs/heads/master@{#38007}

[jochen (gone - plz use gerrit), 2016-07-25 06:53:13]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 06:53:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 06:58:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 06:58:15]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/11105)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/11075)

[jochen (gone - plz use gerrit), 2016-07-25 07:41:00]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 07:41:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 07:46:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 07:46:13]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/11079)

[jochen (gone - plz use gerrit), 2016-07-25 07:53:10]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 07:53:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2016-07-25 07:58:48]

jochen@chromium.org changed reviewers:
+ rossberg@chromium.org

[jochen (gone - plz use gerrit), 2016-07-25 07:58:48]

+rossberg for wasm/ and parsing/

[jochen (gone - plz use gerrit), 2016-07-25 08:10:40]

jochen@chromium.org changed reviewers:
+ ishell@chromium.org

[jochen (gone - plz use gerrit), 2016-07-25 08:10:41]

+Igor, can you please review the entire CL?

it's pretty mechanical, except for the wasm::Result thing that requires a move
constructor to correctly handle ownership

[rossberg, 2016-07-25 08:11:29]

parsing/ and wasm/ LGTM

[rossberg, 2016-07-25 08:11:29]

parsing/ and wasm/ LGTM

[commit-bot: I haz the power, 2016-07-25 08:14:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 08:14:26]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2016-07-25 08:56:45]

nits:

https://codereview.chromium.org/2173403002/diff/40001/src/compiler/graph-visu...
File src/compiler/graph-visualizer.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/src/compiler/graph-visu...
src/compiler/graph-visualizer.cc:74: return std::unique_ptr<const
char[]>(const_cast<const char*>(buffer));
I think const_cast<> is intended to be used to remove constness/volatileness. It
should be possible to pass char* without any casts to where const char* is
expected.

And since we always create a new buffer on every call there's probably no need
to return const array. Maybe it makes sense to return std::unique_ptr<char[]>
from here as we do in all other places.

https://codereview.chromium.org/2173403002/diff/40001/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/src/disassembler.cc#new...
src/disassembler.cc:190: std::unique_ptr<const char[]> obj_name =
accumulator.ToCString();
Same here.

https://codereview.chromium.org/2173403002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:10420: return std::unique_ptr<char[]>(nullptr);
return std::unique_ptr<char[]>() ?

https://codereview.chromium.org/2173403002/diff/40001/src/string-stream.cc
File src/string-stream.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/src/string-stream.cc#ne...
src/string-stream.cc:258: return std::unique_ptr<const char[]>(const_cast<const
char*>(str));
Same here.

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/decoder.h
File src/wasm/decoder.h (right):

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:301: error_msg_.reset(nullptr);
Just .reset()

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/wasm-result.h
File src/wasm/wasm-result.h (right):

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/wasm-result.h#...
src/wasm/wasm-result.h:45: error_msg.reset(nullptr);
Not necessary. Default constructor already does that.

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/wasm-result.h#...
src/wasm/wasm-result.h:49: val = other.val;
How about using std::move(*this, other) here and inlining CopyFrom to the
operator= below?

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/wasm-result.h#...
src/wasm/wasm-result.h:68: void CopyFrom(Result<V>& that) {
Once we use unique_ptr it is no longer a CopyFrom but probably a MoveFrom. WDYT?

https://codereview.chromium.org/2173403002/diff/40001/test/cctest/compiler/te...
File test/cctest/compiler/test-operator.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/test/cctest/compiler/te...
test/cctest/compiler/test-operator.cc:75: const_cast<const
char*>(StrDup(os.str().c_str())));
Unnecessary const_cast and probably constness of a returned array.

[jochen (gone - plz use gerrit), 2016-07-25 09:13:20]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 09:13:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2016-07-25 09:14:49]

https://codereview.chromium.org/2173403002/diff/40001/src/compiler/graph-visu...
File src/compiler/graph-visualizer.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/src/compiler/graph-visu...
src/compiler/graph-visualizer.cc:74: return std::unique_ptr<const
char[]>(const_cast<const char*>(buffer));
On 2016/07/25 at 08:56:44, Igor Sheludko wrote:
> I think const_cast<> is intended to be used to remove constness/volatileness.
It should be possible to pass char* without any casts to where const char* is
expected.
> 
> And since we always create a new buffer on every call there's probably no need
to return const array. Maybe it makes sense to return std::unique_ptr<char[]>
from here as we do in all other places.

done

https://codereview.chromium.org/2173403002/diff/40001/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/src/disassembler.cc#new...
src/disassembler.cc:190: std::unique_ptr<const char[]> obj_name =
accumulator.ToCString();
On 2016/07/25 at 08:56:44, Igor Sheludko wrote:
> Same here.

done

https://codereview.chromium.org/2173403002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:10420: return std::unique_ptr<char[]>(nullptr);
On 2016/07/25 at 08:56:44, Igor Sheludko wrote:
> return std::unique_ptr<char[]>() ?

done

https://codereview.chromium.org/2173403002/diff/40001/src/string-stream.cc
File src/string-stream.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/src/string-stream.cc#ne...
src/string-stream.cc:258: return std::unique_ptr<const char[]>(const_cast<const
char*>(str));
On 2016/07/25 at 08:56:44, Igor Sheludko wrote:
> Same here.

done

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/decoder.h
File src/wasm/decoder.h (right):

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/decoder.h#newc...
src/wasm/decoder.h:301: error_msg_.reset(nullptr);
On 2016/07/25 at 08:56:44, Igor Sheludko wrote:
> Just .reset()

done

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/wasm-result.h
File src/wasm/wasm-result.h (right):

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/wasm-result.h#...
src/wasm/wasm-result.h:45: error_msg.reset(nullptr);
On 2016/07/25 at 08:56:44, Igor Sheludko wrote:
> Not necessary. Default constructor already does that.

done

https://codereview.chromium.org/2173403002/diff/40001/src/wasm/wasm-result.h#...
src/wasm/wasm-result.h:49: val = other.val;
On 2016/07/25 at 08:56:44, Igor Sheludko wrote:
> How about using std::move(*this, other) here and inlining CopyFrom to the
operator= below?

done.

I didn't inline CopyFrom (now MoveFrom), as it is used to copy around results
with incompatible template args

https://codereview.chromium.org/2173403002/diff/40001/test/cctest/compiler/te...
File test/cctest/compiler/test-operator.cc (right):

https://codereview.chromium.org/2173403002/diff/40001/test/cctest/compiler/te...
test/cctest/compiler/test-operator.cc:75: const_cast<const
char*>(StrDup(os.str().c_str())));
On 2016/07/25 at 08:56:44, Igor Sheludko wrote:
> Unnecessary const_cast and probably constness of a returned array.

done

[Igor Sheludko, 2016-07-25 09:37:32]

lgtm once this comment is addressed:

https://codereview.chromium.org/2173403002/diff/60001/src/wasm/wasm-result.h
File src/wasm/wasm-result.h (right):

https://codereview.chromium.org/2173403002/diff/60001/src/wasm/wasm-result.h#...
src/wasm/wasm-result.h:68: error_msg.swap(that.error_msg);
Since "that" is probably going to live for a while
  error_msg = std::move(that.error_msg);
is more correct here.

[commit-bot: I haz the power, 2016-07-25 09:38:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 09:38:40]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-07-25 09:53:08]

LGTM (rubber-stamped)

[jochen (gone - plz use gerrit), 2016-07-25 09:54:55]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-07-25 09:54:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2173403002/#ps80001
(title: "updates")

[commit-bot: I haz the power, 2016-07-25 09:55:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 10:24:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-25 10:27:54]

Description was changed from

==========
Replace SmartArrayPointer<T> with unique_ptr<T[]>

R=bmeurer@chromium.org
BUG=
==========

to

==========
Replace SmartArrayPointer<T> with unique_ptr<T[]>

R=bmeurer@chromium.org
BUG=

Committed: https://crrev.com/37ba8f961bd4e18815f7c56683ed1ea4d2695b7d
Cr-Commit-Position: refs/heads/master@{#38007}
==========

[commit-bot: I haz the power, 2016-07-25 10:27:55]

Patchset 5 (id:??) landed as
https://crrev.com/37ba8f961bd4e18815f7c56683ed1ea4d2695b7d
Cr-Commit-Position: refs/heads/master@{#38007}

[jochen (gone - plz use gerrit), 2016-07-25 10:35:08]

https://codereview.chromium.org/2173403002/diff/60001/src/wasm/wasm-result.h
File src/wasm/wasm-result.h (right):

https://codereview.chromium.org/2173403002/diff/60001/src/wasm/wasm-result.h#...
src/wasm/wasm-result.h:68: error_msg.swap(that.error_msg);
On 2016/07/25 at 09:37:32, Igor Sheludko wrote:
> Since "that" is probably going to live for a while
>   error_msg = std::move(that.error_msg);
> is more correct here.

done