Return unique_ptr from CPDF_Object::Clone().

Return unique_ptr from CPDF_Object::Clone().

Because that's what clone does.  There are numerous
release() calls that will go away as more code is
converted.

Committed: https://pdfium.googlesource.com/pdfium/+/335cf093231c984a23cb9ea113148ea1f19621ba

[Tom Sepez, 2016-11-07 23:29:02]

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

[commit-bot: I haz the power, 2016-11-07 23:29:08]

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-11-07 23:31:37]

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

[commit-bot: I haz the power, 2016-11-07 23:31:38]

Dry run: Try jobs failed on following builders:
  linux_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED, no build URL)
  mac_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED, no build URL)
  win_xfa_32 on master.tryserver.client.pdfium (JOB_FAILED, no build URL)
  win_xfa_clang_32 on master.tryserver.client.pdfium (JOB_FAILED, no build URL)
  win_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED, no build URL)

[Tom Sepez, 2016-11-07 23:42:28]

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

[commit-bot: I haz the power, 2016-11-07 23:42:38]

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-11-07 23:43:07]

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

[commit-bot: I haz the power, 2016-11-07 23:43:08]

Dry run: Try jobs failed on following builders:
  linux_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED, no build URL)
  mac_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED, no build URL)
  win_xfa_32 on master.tryserver.client.pdfium (JOB_FAILED, no build URL)
  win_xfa_clang_32 on master.tryserver.client.pdfium (JOB_FAILED, no build URL)
  win_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED, no build URL)

[Tom Sepez, 2016-11-08 00:04:27]

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

[commit-bot: I haz the power, 2016-11-08 00:04:35]

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-11-08 02:49:25]

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

[commit-bot: I haz the power, 2016-11-08 02:49:26]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-11-08 17:21:40]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2016-11-08 17:21:41]

Lei, ready for review.

[Lei Zhang, 2016-11-08 18:44:46]

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

[commit-bot: I haz the power, 2016-11-08 18:44:56]

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

[Lei Zhang, 2016-11-08 18:55:43]

lgtm

https://codereview.chromium.org/2484033002/diff/60001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_dictionary.cpp (right):

https://codereview.chromium.org/2484033002/diff/60001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_dictionary.cpp:76: return
std::unique_ptr<CPDF_Object>(std::move(pCopy));
I saw you had green trybots on patch set 3, but you explicitly the std::move().
It that really necessary?

https://codereview.chromium.org/2484033002/diff/60001/fpdfsdk/fpdfppo.cpp
File fpdfsdk/fpdfppo.cpp (right):

https://codereview.chromium.org/2484033002/diff/60001/fpdfsdk/fpdfppo.cpp#new...
fpdfsdk/fpdfppo.cpp:279: if (!pClone)
BTW, there's a few places where we check for cloning failures, but I don't think
that can happen.

https://codereview.chromium.org/2484033002/diff/60001/fpdfsdk/fpdfppo.cpp#new...
fpdfsdk/fpdfppo.cpp:296: pClone.release();  // TODO(tsepez): figure out
ownership.
On line 291 , AddIndirectObjet() is suppose to take it?

[commit-bot: I haz the power, 2016-11-08 19:42:51]

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

[commit-bot: I haz the power, 2016-11-08 19:42:52]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-11-09 17:51:36]

On 2016/11/08 18:55:43, Lei Zhang (slow) wrote:
> lgtm
> 
>
https://codereview.chromium.org/2484033002/diff/60001/core/fpdfapi/parser/cpd...
> File core/fpdfapi/parser/cpdf_dictionary.cpp (right):
> 
>
https://codereview.chromium.org/2484033002/diff/60001/core/fpdfapi/parser/cpd...
> core/fpdfapi/parser/cpdf_dictionary.cpp:76: return
> std::unique_ptr<CPDF_Object>(std::move(pCopy));
> I saw you had green trybots on patch set 3, but you explicitly the
std::move().
> It that really necessary?
> 
> https://codereview.chromium.org/2484033002/diff/60001/fpdfsdk/fpdfppo.cpp
> File fpdfsdk/fpdfppo.cpp (right):
> 
>
https://codereview.chromium.org/2484033002/diff/60001/fpdfsdk/fpdfppo.cpp#new...
> fpdfsdk/fpdfppo.cpp:279: if (!pClone)
> BTW, there's a few places where we check for cloning failures, but I don't
think
> that can happen.
> 
>
https://codereview.chromium.org/2484033002/diff/60001/fpdfsdk/fpdfppo.cpp#new...
> fpdfsdk/fpdfppo.cpp:296: pClone.release();  // TODO(tsepez): figure out
> ownership.
> On line 291 , AddIndirectObjet() is suppose to take it?

[Tom Sepez, 2016-11-09 17:51:56]

Uh, ignore that, stray click.

[Tom Sepez, 2016-11-09 18:20:46]

https://codereview.chromium.org/2484033002/diff/60001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_dictionary.cpp (right):

https://codereview.chromium.org/2484033002/diff/60001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_dictionary.cpp:76: return
std::unique_ptr<CPDF_Object>(std::move(pCopy));
On 2016/11/08 18:55:43, Lei Zhang (slow) wrote:
> I saw you had green trybots on patch set 3, but you explicitly the
std::move().
> It that really necessary?

It appears to be necessary for android, which had an issue in
https://build.chromium.org/p/tryserver.client.pdfium/builders/android/builds/...


we're allowed to return an "unmoved" unique_ptr variable, but not an expression
involving that "ummoved" variable:

This elision of copy/move operations, called copy elision, is permitted [...] in
a return statement in a function with a class return type, when the expression
is the name of a non-volatile automatic object with the same cv-unqualified type
as the function return type.

This isn't exactly met by the code, since we're actually returing an unqiue_ptr
to CPDF_Object implicitly constructed from a subclass unique_ptr under the
covers as part of the return.

Which is why we had to write return std::move(pCopy).  But that looks wrong,
generally you want to take advantage of the rule above, and omit the
std::move().  I didn't want someone to remove it, and break stuff.  So I wrote
out the whole expression for clarity.

https://codereview.chromium.org/2484033002/diff/60001/fpdfsdk/fpdfppo.cpp
File fpdfsdk/fpdfppo.cpp (right):

https://codereview.chromium.org/2484033002/diff/60001/fpdfsdk/fpdfppo.cpp#new...
fpdfsdk/fpdfppo.cpp:279: if (!pClone)
On 2016/11/08 18:55:43, Lei Zhang (slow) wrote:
> BTW, there's a few places where we check for cloning failures, but I don't
think
> that can happen.

There's a few places where we return nullptr if we are caught in a cycle, but
that would imply that we've already got some top-level thing, so I think you're
right.  Fixing.

https://codereview.chromium.org/2484033002/diff/60001/fpdfsdk/fpdfppo.cpp#new...
fpdfsdk/fpdfppo.cpp:296: pClone.release();  // TODO(tsepez): figure out
ownership.
On 2016/11/08 18:55:43, Lei Zhang (slow) wrote:
> On line 291 , AddIndirectObjet() is suppose to take it?

Yup.

[Tom Sepez, 2016-11-09 18:28:18]

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

[commit-bot: I haz the power, 2016-11-09 18:28:27]

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

[Lei Zhang, 2016-11-09 19:39:12]

I believe https://www.chromium.org/rvalue-references #11 covers this situation:
"However, if the types of the variable and the return type do not match exactly,
then you will have to use std::move() in order to convert without creating an
extra temporary."

[Tom Sepez, 2016-11-09 19:52:31]

On 2016/11/09 19:39:12, Lei Zhang (slow) wrote:
> I believe https://www.chromium.org/rvalue-references #11 covers this
situation:
> "However, if the types of the variable and the return type do not match
exactly,
> then you will have to use std::move() in order to convert without creating an
> extra temporary."

Agreed. Will bring this back into conformance with that spec.

[Tom Sepez, 2016-11-09 20:02:41]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-11-09 20:02:41]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2484033002/#ps140001
(title: "std::move() it")

[commit-bot: I haz the power, 2016-11-09 20:02:54]

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-11-09 21:28:30]

Description was changed from

==========
Return unique_ptr from CPDF_Object::Clone().

Because that's what clone does.  There are numerous
release() calls that will go away as more code is
converted.
==========

to

==========
Return unique_ptr from CPDF_Object::Clone().

Because that's what clone does.  There are numerous
release() calls that will go away as more code is
converted.

Committed:
https://pdfium.googlesource.com/pdfium/+/335cf093231c984a23cb9ea113148ea1f196...
==========

[commit-bot: I haz the power, 2016-11-09 21:28:32]

Committed patchset #8 (id:140001) as
https://pdfium.googlesource.com/pdfium/+/335cf093231c984a23cb9ea113148ea1f196...