Combine StrToInt methods.

Combine StrToInt methods.

This Cl combines the two StrToInt implementations. In doing so I had to add
some more overrides to toDecimalDigit() and add a isDecimalDigit().

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

Committed: https://pdfium.googlesource.com/pdfium/+/1c91537c9f9669246713a5be628493ae2fc4899a

[dsinclair, 2016-03-03 14:22:29]

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

[dsinclair, 2016-03-03 14:22:30]

PTAL.

[Tom Sepez, 2016-03-03 19:26:47]

https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h...
core/include/fxcrt/fx_ext.h:64: inline int FXSYS_toDecimalDigit(const uint8_t c)
{
I worry about duplicate definitions if we hit a platform where FX_CHAR is
uint8_t or FX_WCHAR is int.

https://codereview.chromium.org/1757283002/diff/1/core/src/fxcrt/fx_basic_gcc...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/1757283002/diff/1/core/src/fxcrt/fx_basic_gcc...
core/src/fxcrt/fx_basic_gcc.cpp:16: bool neg = false;
nit: move to line 20. maybe
bool neg = std::numeric_limits<IntType>::is_signed && *str == '-';
if (neg)
  str++;

[dsinclair, 2016-03-03 19:36:12]

https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h...
core/include/fxcrt/fx_ext.h:64: inline int FXSYS_toDecimalDigit(const uint8_t c)
{
On 2016/03/03 19:26:47, Tom Sepez wrote:
> I worry about duplicate definitions if we hit a platform where FX_CHAR is
> uint8_t or FX_WCHAR is int.

So, if I try to compile without these I get errors that it's ambiguous which
method to choose for uint8_t and int. Without these, I'll need to cast on the
calling side to FX_CHAR, would that be better?

[dsinclair, 2016-03-03 20:06:50]

https://codereview.chromium.org/1757283002/diff/1/core/src/fxcrt/fx_basic_gcc...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/1757283002/diff/1/core/src/fxcrt/fx_basic_gcc...
core/src/fxcrt/fx_basic_gcc.cpp:16: bool neg = false;
On 2016/03/03 19:26:47, Tom Sepez wrote:
> nit: move to line 20. maybe
> bool neg = std::numeric_limits<IntType>::is_signed && *str == '-';
> if (neg)
>   str++;

Done.

[Tom Sepez, 2016-03-03 20:14:58]

https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h...
core/include/fxcrt/fx_ext.h:64: inline int FXSYS_toDecimalDigit(const uint8_t c)
{
On 2016/03/03 19:36:12, dsinclair wrote:
> On 2016/03/03 19:26:47, Tom Sepez wrote:
> > I worry about duplicate definitions if we hit a platform where FX_CHAR is
> > uint8_t or FX_WCHAR is int.
> 
> So, if I try to compile without these I get errors that it's ambiguous which
> method to choose for uint8_t and int. Without these, I'll need to cast on the
> calling side to FX_CHAR, would that be better?
Yes, that would be clearer and safer.

[Tom Sepez, 2016-03-03 20:15:27]

On 2016/03/03 20:14:58, Tom Sepez wrote:
> https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h
> File core/include/fxcrt/fx_ext.h (right):
> 
>
https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h...
> core/include/fxcrt/fx_ext.h:64: inline int FXSYS_toDecimalDigit(const uint8_t
c)
> {
> On 2016/03/03 19:36:12, dsinclair wrote:
> > On 2016/03/03 19:26:47, Tom Sepez wrote:
> > > I worry about duplicate definitions if we hit a platform where FX_CHAR is
> > > uint8_t or FX_WCHAR is int.
> > 
> > So, if I try to compile without these I get errors that it's ambiguous which
> > method to choose for uint8_t and int. Without these, I'll need to cast on
the
> > calling side to FX_CHAR, would that be better?
> Yes, that would be clearer and safer.

But you probably want to cast to Wchar.

[dsinclair, 2016-03-03 20:26:01]

https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1757283002/diff/1/core/include/fxcrt/fx_ext.h...
core/include/fxcrt/fx_ext.h:64: inline int FXSYS_toDecimalDigit(const uint8_t c)
{
On 2016/03/03 20:14:58, Tom Sepez wrote:
> On 2016/03/03 19:36:12, dsinclair wrote:
> > On 2016/03/03 19:26:47, Tom Sepez wrote:
> > > I worry about duplicate definitions if we hit a platform where FX_CHAR is
> > > uint8_t or FX_WCHAR is int.
> > 
> > So, if I try to compile without these I get errors that it's ambiguous which
> > method to choose for uint8_t and int. Without these, I'll need to cast on
the
> > calling side to FX_CHAR, would that be better?
> Yes, that would be clearer and safer.
> 

Done.

[Tom Sepez, 2016-03-03 21:43:03]

Forgive me for being picky, this LGTM, fix the others if you want.

https://codereview.chromium.org/1757283002/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (right):

https://codereview.chromium.org/1757283002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:536: int ch =
m_pBuf[m_Pos++];
nit: maybe we want to make this a uint8_t to match m_pBuf, then the cast to
wchar is an obvious widening and we don't worry about negatives flopping around,
etc.

https://codereview.chromium.org/1757283002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:622: ch = m_pBuf[m_Pos++];
Nit: Two of the three operations that this statement performs are not needed,
right?

https://codereview.chromium.org/1757283002/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/util.cpp (right):

https://codereview.chromium.org/1757283002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/util.cpp:520: /* skip whitespace */
nit: I'd feel better here if we immediately created a uint8_t* pointer to use in
place of nptr.  Then |c| can be a uint8_t, we can lose all the casts to ints,
and we only ever widen to wchar as before.

[dsinclair, 2016-03-03 22:01:43]

PTAL at diff from #3 to #4.

https://codereview.chromium.org/1757283002/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (right):

https://codereview.chromium.org/1757283002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:536: int ch =
m_pBuf[m_Pos++];
On 2016/03/03 21:43:03, Tom Sepez wrote:
> nit: maybe we want to make this a uint8_t to match m_pBuf, then the cast to
> wchar is an obvious widening and we don't worry about negatives flopping
around,
> etc.

Done.

https://codereview.chromium.org/1757283002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:622: ch = m_pBuf[m_Pos++];
On 2016/03/03 21:43:03, Tom Sepez wrote:
> Nit: Two of the three operations that this statement performs are not needed,
> right?

Done.

https://codereview.chromium.org/1757283002/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/util.cpp (right):

https://codereview.chromium.org/1757283002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/util.cpp:520: /* skip whitespace */
On 2016/03/03 21:43:03, Tom Sepez wrote:
> nit: I'd feel better here if we immediately created a uint8_t* pointer to use
in
> place of nptr.  Then |c| can be a uint8_t, we can lose all the casts to ints,
> and we only ever widen to wchar as before.

I'll do one better and just delete the whole method. No errors on removal and,
plus, it should really be using FXSYS_atoi64 instead of reinventing the wheel in
a different shape.

[Tom Sepez, 2016-03-03 22:10:31]

++LGTM

https://codereview.chromium.org/1757283002/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/util.cpp (right):

https://codereview.chromium.org/1757283002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/util.cpp:520: /* skip whitespace */
On 2016/03/03 22:01:43, dsinclair wrote:
> On 2016/03/03 21:43:03, Tom Sepez wrote:
> > nit: I'd feel better here if we immediately created a uint8_t* pointer to
use
> in
> > place of nptr.  Then |c| can be a uint8_t, we can lose all the casts to
ints,
> > and we only ever widen to wchar as before.
> 
> I'll do one better and just delete the whole method. No errors on removal and,
> plus, it should really be using FXSYS_atoi64 instead of reinventing the wheel
in
> a different shape.

Acknowledged.

[dsinclair, 2016-03-03 22:13:00]

Description was changed from

==========
Combine StrToInt methods.

This Cl combines the two StrToInt implementations. In doing so I had to add
some more overrides to toDecimalDigit() and add a isDecimalDigit().

BUG=pdfium:423
==========

to

==========
Combine StrToInt methods.

This Cl combines the two StrToInt implementations. In doing so I had to add
some more overrides to toDecimalDigit() and add a isDecimalDigit().

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

Committed:
https://pdfium.googlesource.com/pdfium/+/1c91537c9f9669246713a5be628493ae2fc4...
==========

[dsinclair, 2016-03-03 22:13:02]

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