Add iterators and unit tests for CPDF_Array class

Add iterators and unit tests for CPDF_Array class

Add iterators to enable range based for loop. Also add unit tests for recent modified functions.

Committed: https://pdfium.googlesource.com/pdfium/+/cdce75706de7e76e73550a0582e1fd28af324fd0

[Wei Li, 2016-04-11 21:31:12]

Description was changed from

==========
Use smart pointers and iterators in CPDF_Array

For internal storage of CPDF_Array, use std::unique_ptr type instead; Add
iterators to the class, and more unit tests to test this class.
==========

to

==========
Add iterators in CPDF_Array

Also add unit tests for this class.
==========

[Wei Li, 2016-04-11 21:31:55]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-04-11 21:32:37]

Description was changed from

==========
Add iterators in CPDF_Array

Also add unit tests for this class.
==========

to

==========
Add iterators in CPDF_Array

Also add unit tests for this class.
==========

[Wei Li, 2016-04-11 21:47:20]

Description was changed from

==========
Add iterators in CPDF_Array

Also add unit tests for this class.
==========

to

==========
Add iterators and unit tests for CPDF_Array class

Add iterators to enable range based for loop. Also add unit tests for recent
modified functions.
==========

[Wei Li, 2016-04-11 21:51:17]

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

[Wei Li, 2016-04-11 21:51:18]

PTAL, thanks

[Tom Sepez, 2016-04-12 00:29:18]

https://codereview.chromium.org/1878803002/diff/20001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp (right):

https://codereview.chromium.org/1878803002/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp:96:
EXPECT_EQ(arr->GetIntegerAt(i), arr2->GetIntegerAt(i));
can we also EXPECT that no new objects got created, eg. shallow copy so the
pointers to the objects are the same?

https://codereview.chromium.org/1878803002/diff/20001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/include/cpdf_array.h (right):

https://codereview.chromium.org/1878803002/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/include/cpdf_array.h:19: using Iterator =
std::vector<CPDF_Object*>::iterator;
These should be called iterator and const_iterator to match the STL convention.

[Wei Li, 2016-04-12 02:56:20]

thanks for the reivew

https://codereview.chromium.org/1878803002/diff/20001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp (right):

https://codereview.chromium.org/1878803002/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp:96:
EXPECT_EQ(arr->GetIntegerAt(i), arr2->GetIntegerAt(i));
On 2016/04/12 00:29:17, Tom Sepez wrote:
> can we also EXPECT that no new objects got created, eg. shallow copy so the
> pointers to the objects are the same?

Turns out it is a bit more complicated :)

https://codereview.chromium.org/1878803002/diff/20001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/include/cpdf_array.h (right):

https://codereview.chromium.org/1878803002/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/include/cpdf_array.h:19: using Iterator =
std::vector<CPDF_Object*>::iterator;
On 2016/04/12 00:29:17, Tom Sepez wrote:
> These should be called iterator and const_iterator to match the STL
convention.

Done.

[Tom Sepez, 2016-04-12 16:34:44]

lgtm

https://codereview.chromium.org/1878803002/diff/40001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp (right):

https://codereview.chromium.org/1878803002/diff/40001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp:127: // Shallow copy of
references means just creating new references
nit: Ok, lets remove the words shallow (and deep if it appears anywhere) since
it's always a deep copy of the objects (to my way of thinking) plus a possible
de-referencing based on the flag.  Can we use the term dereferencing ?

[Wei Li, 2016-04-13 19:57:55]

https://codereview.chromium.org/1878803002/diff/40001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp (right):

https://codereview.chromium.org/1878803002/diff/40001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp:127: // Shallow copy of
references means just creating new references
On 2016/04/12 16:34:44, Tom Sepez wrote:
> nit: Ok, lets remove the words shallow (and deep if it appears anywhere) since
> it's always a deep copy of the objects (to my way of thinking) plus a possible
> de-referencing based on the flag.  Can we use the term dereferencing ?

Done.

[Wei Li, 2016-04-13 21:28:40]

The CQ bit was checked by weili@chromium.org

[Wei Li, 2016-04-13 21:28:40]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1878803002/#ps60001
(title: "Comment change")

[commit-bot: I haz the power, 2016-04-13 21:28:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1878803002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1878803002/60001

[commit-bot: I haz the power, 2016-04-13 21:40:13]

Description was changed from

==========
Add iterators and unit tests for CPDF_Array class

Add iterators to enable range based for loop. Also add unit tests for recent
modified functions.
==========

to

==========
Add iterators and unit tests for CPDF_Array class

Add iterators to enable range based for loop. Also add unit tests for recent
modified functions.

Committed:
https://pdfium.googlesource.com/pdfium/+/cdce75706de7e76e73550a0582e1fd28af32...
==========

[commit-bot: I haz the power, 2016-04-13 21:40:14]

Committed patchset #3 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/cdce75706de7e76e73550a0582e1fd28af32...