Document non-null entries for CPDF_Dictionary

Then remove one small redundant check.

-- Avoid assignment in conditional while at it.

Change-Id: I885892b8a8ed40dffeb555da035f07252feaee92
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/87412
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>

core/fpdfapi/parser/cpdf_dictionary.cpp

diff --git a/core/fpdfapi/parser/cpdf_dictionary.cpp b/core/fpdfapi/parser/cpdf_dictionary.cpp
index a798b6e..fc671e7 100644
--- a/core/fpdfapi/parser/cpdf_dictionary.cpp
+++ b/core/fpdfapi/parser/cpdf_dictionary.cpp
@@ -33,7 +33,7 @@
   // and break cyclic references.
   m_ObjNum = kInvalidObjNum;
   for (auto& it : m_Map) {
-    if (it.second && it.second->GetObjNum() == kInvalidObjNum)
+    if (it.second->GetObjNum() == kInvalidObjNum)
       it.second.Leak();
   }
 }
@@ -75,7 +75,8 @@
   for (const auto& it : locker) {
     if (!pdfium::Contains(*pVisited, it.second.Get())) {
       std::set<const CPDF_Object*> visited(*pVisited);
-      if (auto obj = it.second->CloneNonCyclic(bDirect, &visited))
+      auto obj = it.second->CloneNonCyclic(bDirect, &visited);
+      if (obj)
         pCopy->m_Map.insert(std::make_pair(it.first, std::move(obj)));
     }
   }

core/fpdfapi/parser/cpdf_dictionary.h

diff --git a/core/fpdfapi/parser/cpdf_dictionary.h b/core/fpdfapi/parser/cpdf_dictionary.h
index a7d303f..d8ba1c0 100644
--- a/core/fpdfapi/parser/cpdf_dictionary.h
+++ b/core/fpdfapi/parser/cpdf_dictionary.h
@@ -22,6 +22,8 @@
 
 class CPDF_IndirectObjectHolder;
 
+// Dictionaries never contain nullptr for valid keys, but some of the methods
+// will return nullptr to indicate non-existent keys.
 class CPDF_Dictionary final : public CPDF_Object {
  public:
   using const_iterator =
@@ -77,9 +79,10 @@
   std::vector<ByteString> GetKeys() const;
 
   // Creates a new object owned by the dictionary and returns an unowned
-  // pointer to it. Prefer using these templates over calls to SetFor(),
-  // since by creating a new object with no previous references, they ensure
-  // cycles can not be introduced.
+  // pointer to it. Invalidates iterators for the element with the key |key|.
+  // Prefer using these templates over calls to SetFor(), since by creating
+  // a new object with no previous references, they ensure cycles can not be
+  // introduced.
   template <typename T, typename... Args>
   typename std::enable_if<!CanInternStrings<T>::value, T*>::type SetNewFor(
       const ByteString& key,
@@ -95,14 +98,15 @@
         key, pdfium::MakeRetain<T>(m_pPool, std::forward<Args>(args)...)));
   }
 
+  // If |pObj| is null, then |key| is erased from the map. Otherwise, takes
+  // ownership of |pObj|, returns an unowned pointer to it. Invalidates
+  // iterators for the element with the key |key|.
+  CPDF_Object* SetFor(const ByteString& key, RetainPtr<CPDF_Object> pObj);
+
   // Convenience functions to convert native objects to array form.
   void SetRectFor(const ByteString& key, const CFX_FloatRect& rect);
   void SetMatrixFor(const ByteString& key, const CFX_Matrix& matrix);
 
-  // Set* functions invalidate iterators for the element with the key |key|.
-  // Takes ownership of |pObj|, returns an unowned pointer to it.
-  CPDF_Object* SetFor(const ByteString& key, RetainPtr<CPDF_Object> pObj);
-
   void ConvertToIndirectObjectFor(const ByteString& key,
                                   CPDF_IndirectObjectHolder* pHolder);