Remove CFX_SegmentedArray use from master

Remove CFX_SegmentedArray use from master.

Replace with std::deque.
Make member naming more consistent.

R=ochang@chromium.org, thestig@chromium.org

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

[Tom Sepez, 2016-01-22 19:58:48]

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

[Tom Sepez, 2016-01-22 19:58:49]

Pls. review.

[Tom Sepez, 2016-01-22 19:59:10]

Description was changed from

==========
Remove CFX_SegmentedArray use from master
==========

to

==========
Remove CFX_SegmentedArray use from master.

Replace with std::deque.
Make member naming more consistent.
==========

[Oliver Chang, 2016-01-22 20:17:56]

Could we change these

int var = m_Deque.size(); 

to "size_t var" = where we can?

MSVC gives warnings for these too, don't they?

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
core/src/fpdftext/fpdf_text_int.cpp:369: int nCount = m_CharList.size();
could we change this to a size_t?

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
core/src/fpdftext/fpdf_text_int.cpp:410: int nCount = m_CharList.size();
ditto, many more instances in this file.

[Tom Sepez, 2016-01-22 20:41:08]

On 2016/01/22 20:17:56, Oliver Chang wrote:
> Could we change these
> 
> int var = m_Deque.size(); 
> 
> to "size_t var" = where we can?
> 
> MSVC gives warnings for these too, don't they?
Good question, I'm not sure.
 
>
https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
> File core/src/fpdftext/fpdf_text_int.cpp (right):
> 
>
https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
> core/src/fpdftext/fpdf_text_int.cpp:369: int nCount = m_CharList.size();
> could we change this to a size_t?
> 
>
https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
> core/src/fpdftext/fpdf_text_int.cpp:410: int nCount = m_CharList.size();
> ditto, many more instances in this file.
Not clear which is worse -- since these indices are also used with other FX
types that don't understand size_t.  If MSVC is giving trunctation warnings,
then we'd need to add casts in a bunch of places ...

[Tom Sepez, 2016-01-22 20:52:05]

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
core/src/fpdftext/fpdf_text_int.cpp:184: m_CharIndex.SetAt(indexSize - 1, i +
1);
For instance, m_CharIndex is a "word array" (array of uint16_t), so the
assignment of i+1 (int) won't fit beyond 64k.

[Oliver Chang, 2016-01-22 21:08:07]

On 2016/01/22 20:41:08, Tom Sepez wrote:
> On 2016/01/22 20:17:56, Oliver Chang wrote:
> > Could we change these
> > 
> > int var = m_Deque.size(); 
> > 
> > to "size_t var" = where we can?
> > 
> > MSVC gives warnings for these too, don't they?
> Good question, I'm not sure.

Hmm, based on
https://pdfium.googlesource.com/pdfium/+/b29338d126125d96d63817af1d80a64ea929...,
I would assume it does (unless these warnings are somehow disabled here)?

[Tom Sepez, 2016-01-22 21:23:41]

On 2016/01/22 21:08:07, Oliver Chang wrote:
> On 2016/01/22 20:41:08, Tom Sepez wrote:
> > On 2016/01/22 20:17:56, Oliver Chang wrote:
> > > Could we change these
> > > 
> > > int var = m_Deque.size(); 
> > > 
> > > to "size_t var" = where we can?
> > > 
> > > MSVC gives warnings for these too, don't they?
> > Good question, I'm not sure.
> 
> Hmm, based on
>
https://pdfium.googlesource.com/pdfium/+/b29338d126125d96d63817af1d80a64ea929...,
> I would assume it does (unless these warnings are somehow disabled here)?

Ayup.  So at a min, it needs casts prior to all assignments.  Yuck.

[Oliver Chang, 2016-01-22 21:26:24]

> Not clear which is worse -- since these indices are also used with other FX
types that don't understand size_t.  If MSVC is giving trunctation warnings,
then we'd need to add casts in a bunch of places ...

Ah, I was thinking about the more simple cases, where we can just avoid a
cast... e.g. (see below)

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
core/src/fpdftext/fpdf_text_int.cpp:1973: int nCount = m_CharList.size();
this can probably be just a size_t instead of being casted.

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
core/src/fpdftext/fpdf_text_int.cpp:2046: int size = m_TempCharList.size();
this one too

[Tom Sepez, 2016-01-22 21:41:56]

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
core/src/fpdftext/fpdf_text_int.cpp:1973: int nCount = m_CharList.size();
On 2016/01/22 21:26:23, Oliver Chang wrote:
> this can probably be just a size_t instead of being casted.

Acknowledged.

https://codereview.chromium.org/1618273004/diff/1/core/src/fpdftext/fpdf_text...
core/src/fpdftext/fpdf_text_int.cpp:2046: int size = m_TempCharList.size();
On 2016/01/22 21:26:23, Oliver Chang wrote:
> this one too

Acknowledged.

[Tom Sepez, 2016-01-22 23:52:14]

See PS#2.

[Oliver Chang, 2016-01-23 00:02:25]

awesome, lgtm.

[Tom Sepez, 2016-01-23 00:03:05]

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

[Tom Sepez, 2016-01-23 00:03:48]

On 2016/01/23 00:02:25, Oliver Chang wrote:
> awesome, lgtm.

Thanks. Also +thestig (000) who may someday not be OOO, to get another pair of
eyeballs on any typos I might have made.

[Tom Sepez, 2016-01-23 00:07:56]

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:481: int startIndex = start;
nit: someday, someone should make this loop sane.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:490: startIndex++;
nit: uh huh, postincrement then assign then never use again.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:504: endIndex--;
nit: then postdecrement, then add 1 on the next line.

[Lei Zhang, 2016-01-25 22:11:16]

lgtm

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:163: PAGECHAR_INFO charinfo = m_CharList[i];
improvement over original code: const ref?

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:455: const PAGECHAR_INFO* charinfo =
&m_CharList[index];
Also const ref?

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:756: PAGECHAR_INFO charinfo =
m_CharList[index];
const ref

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:973: PAGECHAR_INFO Info = m_TempCharList[i];
Would you mind renaming |Info| and |Info2| while you are here?

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:1005: PAGECHAR_INFO Info =
m_TempCharList[i];
Ditto.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:1538: PAGECHAR_INFO* cha =
&m_TempCharList.back();
rename |cha| ?

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:1691: PAGECHAR_INFO* charinfo1 =
&m_TempCharList[n - 1];
const ref instead?

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:1776: PAGECHAR_INFO preChar;
cosnt PAGECHAR_INFO* ?

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2032: PAGECHAR_INFO preChar;
Ditto, const PAGECHAR_INFO*

[Tom Sepez, 2016-01-25 23:04:47]

fpdf_text_int.cpp need re-review, icky merge forward.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:163: PAGECHAR_INFO charinfo = m_CharList[i];
On 2016/01/25 22:11:16, Lei Zhang wrote:
> improvement over original code: const ref?

Done.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:455: const PAGECHAR_INFO* charinfo =
&m_CharList[index];
On 2016/01/25 22:11:16, Lei Zhang wrote:
> Also const ref?

Done.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:756: PAGECHAR_INFO charinfo =
m_CharList[index];
On 2016/01/25 22:11:15, Lei Zhang wrote:
> const ref

Done.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:973: PAGECHAR_INFO Info = m_TempCharList[i];
On 2016/01/25 22:11:15, Lei Zhang wrote:
> Would you mind renaming |Info| and |Info2| while you are here?

As in starts with lower case? sure.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:1538: PAGECHAR_INFO* cha =
&m_TempCharList.back();
On 2016/01/25 22:11:16, Lei Zhang wrote:
> rename |cha| ?

Done.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:1691: PAGECHAR_INFO* charinfo1 =
&m_TempCharList[n - 1];
On 2016/01/25 22:11:15, Lei Zhang wrote:
> const ref instead?

Done.

https://codereview.chromium.org/1618273004/diff/20001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:1776: PAGECHAR_INFO preChar;
On 2016/01/25 22:11:16, Lei Zhang wrote:
> cosnt PAGECHAR_INFO* ?

Const ptr, also renamed since preChar is also a wchar in another block.

[Lei Zhang, 2016-01-25 23:37:07]

lgtm

[Tom Sepez, 2016-01-25 23:39:04]

Description was changed from

==========
Remove CFX_SegmentedArray use from master.

Replace with std::deque.
Make member naming more consistent.
==========

to

==========
Remove CFX_SegmentedArray use from master.

Replace with std::deque.
Make member naming more consistent.

R=ochang@chromium.org, thestig@chromium.org

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

[Tom Sepez, 2016-01-25 23:39:05]

Committed patchset #5 (id:80001) manually as
c64e4007ee4561ec2ed3ce986191caf9b024ef55 (presubmit successful).