Add type cast definitions for CPDF_Reference.

Add type cast definitions for CPDF_Reference.

This Cl adds ToReference, CPDF_Object::AsReference and CPDF_Object::IsReference
and updates the src to use them as needed.

BUG=pdfium:201
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/9024e026dae1af064b8467bb0f62278417fb82d1

[dsinclair, 2015-10-22 20:01:48]

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

[dsinclair, 2015-10-22 20:01:49]

PTAL.

[Lei Zhang, 2015-10-22 21:26:52]

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_edit/...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_edit/...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:67: const CPDF_Reference* p =
pObj->AsReference();
I think you can just fold |p| into the next line. It should just fit 80 chars /
col. Ditto below.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:679: CPDF_Object* pRes =
nullptr;
move this down to where it's used

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:687: CPDF_Dictionary* pList =
m_pPageResources->GetDict(FX_BSTRC("XObject"));
There's a shadow variable here.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:186: if (IsReference() &&
GetDirect())
Maybe save the GetDirect() return value and use it on the next line?

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2387: if (pLenObj &&
((!pLenObjRef || ((pLenObjRef->GetObjList()) &&
There's some extra parenthesis in there? Consider breaking out some of this into
a boolean variable.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3401: if (m_dwInfoObjNum)
braces

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3507: CPDF_Reference* pKid =
pKids->AsReference();
fold into next line?

[Tom Sepez, 2015-10-22 21:49:14]

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:679: CPDF_Object* pRes =
nullptr;
nit: Move this down and combine with initialization at 691

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:687: CPDF_Dictionary* pList =
m_pPageResources->GetDict(FX_BSTRC("XObject"));
Argh, shadowing of pList at 683!  This would be a bug. Otherwise we segv at 691
when this is present and it goes out of scope.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:691: pRes =
pList->GetElement(name);
nit: maybe 
   CPDF_Reference* pRes = ToReference(pList->GetElement(name));

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:694: 
And I think the whole thing reduces down to this (but doublecheck), and I think
this reads more clearly.

CPDF_Dictionary* pList = m_pResources->GetDict(FX_BSTRC("XObject"));
if (!pList && m_pPageResources && m_pResources != m_pPageResources)
  pList = m_pPageResources->GetDict(FX_BSTRC("XObject"));
if (!pList)
  return;
CPDF_Resource* pRes = ToReference(pList->GetElement(name));
if (!pRes)
  return;

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:696: if
(m_pDocument->IsFormStream(pRes->AsReference()->GetRefObjNum(), bForm) &&
nit: then you can lose the redundant AsReference here.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_document.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_document.cpp:311: if (!pRef)
nit: I think this is easier if you invert the test and combine, eg.
  if (pref && pref->getRefObjNum() == objnum) 
    return true;

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:630: m_Map.Lookup(key,
(void*&)p);
nit: this is one of my pet peeves.  The cast to void* is illegal because C
doesn't assure us that the representation of void pointers and struct pointers
are the same (though it hasn't mattered since some machines went extinct in the
late 80s).

void* result;
m_Map.Lookup(key, result);
CPDF_Object* p = static_cast<CPDF_Object*>(result);

Here the compiler is obligated to manipulate the representation of the pointer
during the cast if it should differ.  Don't fix now, but something to track down
later if you are eager.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:634: if (p)
nit: you can save the duplicate calls to GetUnicodeText if you're willing to
retest p.

if (CPDF_Reference* pRef = ToReference(p))
  p = pRef->GetDirect();
return p ? p->GetUnicodeText(pCharMap) : CFX_WideString();

[dsinclair, 2015-10-26 13:55:45]

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_edit/...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_edit/...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:67: const CPDF_Reference* p =
pObj->AsReference();
On 2015/10/22 21:26:51, Lei Zhang wrote:
> I think you can just fold |p| into the next line. It should just fit 80 chars
/
> col. Ditto below.

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:679: CPDF_Object* pRes =
nullptr;
On 2015/10/22 21:26:51, Lei Zhang wrote:
> move this down to where it's used

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:679: CPDF_Object* pRes =
nullptr;
On 2015/10/22 21:49:14, Tom Sepez wrote:
> nit: Move this down and combine with initialization at 691

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:687: CPDF_Dictionary* pList =
m_pPageResources->GetDict(FX_BSTRC("XObject"));
On 2015/10/22 21:26:51, Lei Zhang wrote:
> There's a shadow variable here.

Mistake in my simplification. Should not have been re-declared.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:691: pRes =
pList->GetElement(name);
On 2015/10/22 21:49:13, Tom Sepez wrote:
> nit: maybe 
>    CPDF_Reference* pRes = ToReference(pList->GetElement(name));

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:694: 
On 2015/10/22 21:49:14, Tom Sepez wrote:
> And I think the whole thing reduces down to this (but doublecheck), and I
think
> this reads more clearly.
> 
> CPDF_Dictionary* pList = m_pResources->GetDict(FX_BSTRC("XObject"));
> if (!pList && m_pPageResources && m_pResources != m_pPageResources)
>   pList = m_pPageResources->GetDict(FX_BSTRC("XObject"));
> if (!pList)
>   return;
> CPDF_Resource* pRes = ToReference(pList->GetElement(name));
> if (!pRes)
>   return;
> 

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_page/...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:696: if
(m_pDocument->IsFormStream(pRes->AsReference()->GetRefObjNum(), bForm) &&
On 2015/10/22 21:49:14, Tom Sepez wrote:
> nit: then you can lose the redundant AsReference here.

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_document.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_document.cpp:311: if (!pRef)
On 2015/10/22 21:49:14, Tom Sepez wrote:
> nit: I think this is easier if you invert the test and combine, eg.
>   if (pref && pref->getRefObjNum() == objnum) 
>     return true;

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:186: if (IsReference() &&
GetDirect())
On 2015/10/22 21:26:51, Lei Zhang wrote:
> Maybe save the GetDirect() return value and use it on the next line?

That requires breaking it into 2 if's I believe which I think reads worse.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:630: m_Map.Lookup(key,
(void*&)p);
On 2015/10/22 21:49:14, Tom Sepez wrote:
> nit: this is one of my pet peeves.  The cast to void* is illegal because C
> doesn't assure us that the representation of void pointers and struct pointers
> are the same (though it hasn't mattered since some machines went extinct in
the
> late 80s).
> 
> void* result;
> m_Map.Lookup(key, result);
> CPDF_Object* p = static_cast<CPDF_Object*>(result);
> 
> Here the compiler is obligated to manipulate the representation of the pointer
> during the cast if it should differ.  Don't fix now, but something to track
down
> later if you are eager.

https://bugs.chromium.org/p/pdfium/issues/detail?id=243

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:634: if (p)
On 2015/10/22 21:49:14, Tom Sepez wrote:
> nit: you can save the duplicate calls to GetUnicodeText if you're willing to
> retest p.
> 
> if (CPDF_Reference* pRef = ToReference(p))
>   p = pRef->GetDirect();
> return p ? p->GetUnicodeText(pCharMap) : CFX_WideString();

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2387: if (pLenObj &&
((!pLenObjRef || ((pLenObjRef->GetObjList()) &&
On 2015/10/22 21:26:51, Lei Zhang wrote:
> There's some extra parenthesis in there? Consider breaking out some of this
into
> a boolean variable.

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3401: if (m_dwInfoObjNum)
On 2015/10/22 21:26:51, Lei Zhang wrote:
> braces

Done.

https://codereview.chromium.org/1420973002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:3507: CPDF_Reference* pKid =
pKids->AsReference();
On 2015/10/22 21:26:51, Lei Zhang wrote:
> fold into next line?

Done.

[Tom Sepez, 2015-10-26 16:12:42]

lgtm

[dsinclair, 2015-10-26 16:51:56]

Committed patchset #2 (id:20001) manually as
9024e026dae1af064b8467bb0f62278417fb82d1 (presubmit successful).