Return unique_ptr from CPDF_Object::Clone().

core/fpdfapi/parser/cpdf_dictionary.cpp

Index: core/fpdfapi/parser/cpdf_dictionary.cpp
diff --git a/core/fpdfapi/parser/cpdf_dictionary.cpp b/core/fpdfapi/parser/cpdf_dictionary.cpp
index 37efbbc34a8afaa839572b813d54433574d8a66c..0035fa53324652b002a24e402294df24e89409e9 100644
--- a/core/fpdfapi/parser/cpdf_dictionary.cpp
+++ b/core/fpdfapi/parser/cpdf_dictionary.cpp
@@ -57,23 +57,23 @@ 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 {