Return unique_ptr from CPDF_Object::Clone().

core/fpdfapi/parser/cpdf_dictionary.cpp

 // Copyright 2016 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "core/fpdfapi/parser/cpdf_dictionary.h"
 
 #include <set>
 #include <utility>
@@ skipping 39 matching lines @@
 }
 
 CPDF_Dictionary* CPDF_Dictionary::AsDictionary() {
   return this;
 }
 
 const CPDF_Dictionary* CPDF_Dictionary::AsDictionary() const {
   return this;
 }
 
-CPDF_Object* CPDF_Dictionary::Clone() const {
+std::unique_ptr<CPDF_Object> CPDF_Dictionary::Clone() const {
   return CloneObjectNonCyclic(false);
 }
 
-CPDF_Object* CPDF_Dictionary::CloneNonCyclic(
+std::unique_ptr<CPDF_Object> CPDF_Dictionary::CloneNonCyclic(
     bool bDirect,
     std::set<const CPDF_Object*>* pVisited) const {
   pVisited->insert(this);
-  CPDF_Dictionary* pCopy = new CPDF_Dictionary(m_pPool);
+  auto pCopy = pdfium::MakeUnique<CPDF_Dictionary>(m_pPool);
   for (const auto& it : *this) {
     CPDF_Object* value = it.second;
     if (!pdfium::ContainsKey(*pVisited, value)) {
-      pCopy->m_Map.insert(
-          std::make_pair(it.first, value->CloneNonCyclic(bDirect, pVisited)));
+      pCopy->m_Map.insert(std::make_pair(
+          it.first, value->CloneNonCyclic(bDirect, pVisited).release()));
     }
   }
-  return pCopy;
+  return std::unique_ptr<CPDF_Object>(std::move(pCopy));

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

I saw you had green trybots on patch set 3, but you explicitly the std::move().
It that really necessary?

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

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.

 }
 
 CPDF_Object* CPDF_Dictionary::GetObjectFor(const CFX_ByteString& key) const {
   auto it = m_Map.find(key);
   return it != m_Map.end() ? it->second : nullptr;
 }
 
 CPDF_Object* CPDF_Dictionary::GetDirectObjectFor(
     const CFX_ByteString& key) const {
   CPDF_Object* p = GetObjectFor(key);
@@ skipping 190 matching lines @@
   pArray->AddNumber(matrix.c);
   pArray->AddNumber(matrix.d);
   pArray->AddNumber(matrix.e);
   pArray->AddNumber(matrix.f);
   SetFor(key, pArray);
 }
 
 CFX_ByteString CPDF_Dictionary::MaybeIntern(const CFX_ByteString& str) {
   return m_pPool ? m_pPool->Intern(str) : str;
 }