Fixed object references in CPDF_Object

Fixed object references in CPDF_Object

This is a regression from commit 90853cb.

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

Committed: https://pdfium.googlesource.com/pdfium/+/7671d304ca4d3aec658c0ac8d5eea4c64e23b144

[Wei Li, 2016-01-22 03:02:57]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-01-22 03:03:02]

Patchset #1 (id:20001) has been deleted

[Wei Li, 2016-01-22 03:10:17]

Description was changed from

==========
Fixed object references in CPDF_Object

BUG=pdfium:365
==========

to

==========
Fixed object references in CPDF_Object

This is a regression from commit 90853cb.

BUG=pdfium:365
==========

[Wei Li, 2016-01-22 03:10:59]

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

[Wei Li, 2016-01-22 03:10:59]

PTAL, thanks

[Tom Sepez, 2016-01-22 17:34:27]

Thanks immensely for writing the test!  The code itself looks good, just some
tidying of the test.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right):

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:46: m_DirectObjs =
{boolean_false_obj, boolean_true_obj, number_int_obj,
Are we allowed to do brace-list initialization of classes now?

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:60: for (int i =
0; i < 7; i++)
We probably should be using the result of each AddIndirectObject call above
rather than i+1 as the object number.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:65: auto it =
m_DirectObjs.back();
nit: local not needed, just m_DirectObjs.back()->Release();

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:78:
std::vector<CPDF_Object*> m_DirectObjs;
nit: could be std::vector<std::unique_ptr<CPDF_Object,
ReleaseDeleter<CPDF_Object>>> and it would be self-cleaning.  Or not.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:80:
CPDF_IndirectObjectHolder* m_ObjHolder;
nit: could be std::unique_ptr and let dtor rather than TearDown() clean it up?

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:103: NULL, NULL,
NULL, NULL, "A simple test", "\t\n", "space",
nit: prefer nullptr to NULL.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:106: for (int i =
0; i < m_DirectObjs.size(); i++) {
nit: prefer ++i (several places in this file).

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:107: if
(!direct_obj_results[i])
nit: need {} since it spans more than one line.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:118: if
(!indirect_obj_results[i])
nit: need {} since it covers more than one physical line.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:120:
indirect_obj_results[i]);
nit: maybe comparision against nullptr rather than indirect_obj_results[i] reads
more clearly.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:129: 0, 0, 0,   
0,        0};
nit: Did clang-format do this? hmm.  weird spacing.

[Wei Li, 2016-01-22 22:26:48]

Patchset #2 (id:60001) has been deleted

[Wei Li, 2016-01-22 22:40:56]

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right):

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:46: m_DirectObjs =
{boolean_false_obj, boolean_true_obj, number_int_obj,
On 2016/01/22 17:34:27, Tom Sepez wrote:
> Are we allowed to do brace-list initialization of classes now? 

Yes, C++11 new feature for containers. But not any more once I change to a
vector of unique_ptrs.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:60: for (int i =
0; i < 7; i++)
On 2016/01/22 17:34:27, Tom Sepez wrote:
> We probably should be using the result of each AddIndirectObject call above
> rather than i+1 as the object number.

Good idea, it is cleaner now.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:65: auto it =
m_DirectObjs.back();
On 2016/01/22 17:34:27, Tom Sepez wrote:
> nit: local not needed, just m_DirectObjs.back()->Release();

Done.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:78:
std::vector<CPDF_Object*> m_DirectObjs;
On 2016/01/22 17:34:27, Tom Sepez wrote:
> nit: could be std::vector<std::unique_ptr<CPDF_Object,
> ReleaseDeleter<CPDF_Object>>> and it would be self-cleaning.  Or not.

Done.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:80:
CPDF_IndirectObjectHolder* m_ObjHolder;
On 2016/01/22 17:34:27, Tom Sepez wrote:
> nit: could be std::unique_ptr and let dtor rather than TearDown() clean it up?

Done.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:103: NULL, NULL,
NULL, NULL, "A simple test", "\t\n", "space",
On 2016/01/22 17:34:27, Tom Sepez wrote:
> nit: prefer nullptr to NULL.

Done.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:106: for (int i =
0; i < m_DirectObjs.size(); i++) {
On 2016/01/22 17:34:27, Tom Sepez wrote:
> nit: prefer ++i (several places in this file).

Done.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:107: if
(!direct_obj_results[i])
On 2016/01/22 17:34:27, Tom Sepez wrote:
> nit: need {} since it spans more than one line.

Done.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:118: if
(!indirect_obj_results[i])
On 2016/01/22 17:34:27, Tom Sepez wrote:
> nit: need {} since it covers more than one physical line.

Done.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:120:
indirect_obj_results[i]);
On 2016/01/22 17:34:27, Tom Sepez wrote:
> nit: maybe comparision against nullptr rather than indirect_obj_results[i]
reads
> more clearly.

Done.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:129: 0, 0, 0,   
0,        0};
On 2016/01/22 17:34:27, Tom Sepez wrote:
> nit: Did clang-format do this? hmm.  weird spacing.

Yes, it always tries to align the elements even I tried to put them in one line.

[Tom Sepez, 2016-01-22 22:54:49]

Excellent. LGTM.

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right):

https://codereview.chromium.org/1617043004/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:46: m_DirectObjs =
{boolean_false_obj, boolean_true_obj, number_int_obj,
On 2016/01/22 22:40:56, Wei Li wrote:
> On 2016/01/22 17:34:27, Tom Sepez wrote:
> > Are we allowed to do brace-list initialization of classes now? 
> 
> Yes, C++11 new feature for containers. But not any more once I change to a
> vector of unique_ptrs.

Acknowledged.

https://codereview.chromium.org/1617043004/diff/100001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right):

https://codereview.chromium.org/1617043004/diff/100001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:71: using
ScopedObj = std::unique_ptr<CPDF_Object, ReleaseDeleter<CPDF_Object>>;
nit: |using|s should be first thing in protected block, then blank line, then
members.

[Wei Li, 2016-01-23 02:33:20]

thanks, will submit.

https://codereview.chromium.org/1617043004/diff/100001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp (right):

https://codereview.chromium.org/1617043004/diff/100001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_parser/fpdf_parser_objects_unittest.cpp:71: using
ScopedObj = std::unique_ptr<CPDF_Object, ReleaseDeleter<CPDF_Object>>;
On 2016/01/22 22:54:49, Tom Sepez wrote:
> nit: |using|s should be first thing in protected block, then blank line, then
> members.

Done.

[Wei Li, 2016-01-23 02:35:51]

Description was changed from

==========
Fixed object references in CPDF_Object

This is a regression from commit 90853cb.

BUG=pdfium:365
==========

to

==========
Fixed object references in CPDF_Object

This is a regression from commit 90853cb.

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

Committed:
https://pdfium.googlesource.com/pdfium/+/7671d304ca4d3aec658c0ac8d5eea4c64e23...
==========

[Wei Li, 2016-01-23 02:35:52]

Committed patchset #4 (id:120001) manually as
7671d304ca4d3aec658c0ac8d5eea4c64e23b144 (presubmit successful).