Revert "Revert "Revert "Revert "Cleanup some numeric code.""""

Revert "Revert "Revert "Revert "Cleanup some numeric code.""""

This reverts commit da06e60fb5a095a91c9a4f509466667878624cb3.

Cleanup some numeric code.

This changes the various comparisons of char >= '0' && char <= '9' and
char < '0' || char > '9' to use std::isdigit checks. It also cleans up
a handful of hex to digit conversions to call one common method.

R=tsepez@chromium.org

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

[dsinclair, 2015-11-03 21:05:48]

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

[dsinclair, 2015-11-03 21:05:48]

PTAL. Fixes windows in PS2.

https://codereview.chromium.org/1433513002/diff/20001/core/include/fxcrt/fx_e...
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1433513002/diff/20001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:46: inline int FXSYS_toDecimalDigit(const FX_WCHAR
c) {
This makes the windows build happy locally. If I try to have two copies, one
taking FX_CHAR and the other FX_WCHAR I get lots of can't figure out which one
to call type errors.

With just FX_WCHAR this builds on windows and linux without issue.

[Tom Sepez, 2015-11-03 21:54:37]

https://codereview.chromium.org/1433513002/diff/20001/core/include/fxcrt/fx_e...
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1433513002/diff/20001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:46: inline int FXSYS_toDecimalDigit(const FX_WCHAR
c) {
On 2015/11/03 21:05:48, dsinclair wrote:
> This makes the windows build happy locally. If I try to have two copies, one
> taking FX_CHAR and the other FX_WCHAR I get lots of can't figure out which one
> to call type errors.
> 
isdigit says: The behavior is undefined if the value of ch is not representable
as unsigned char. And so we may well hit that case with a wide char. Got a
pointer to the windows build?

[Tom Sepez, 2015-11-03 22:02:51]

https://codereview.chromium.org/1433513002/diff/20001/core/include/fxcrt/fx_e...
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1433513002/diff/20001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:46: inline int FXSYS_toDecimalDigit(const FX_WCHAR
c) {
If its
http://build.chromium.org/p/client.pdfium/builders/windows/builds/56/steps/co...

The maybe you need a wchar version that calls std::iswxdigit to go along with
it?

[dsinclair, 2015-11-04 14:45:01]

https://codereview.chromium.org/1433513002/diff/20001/core/include/fxcrt/fx_e...
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1433513002/diff/20001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:46: inline int FXSYS_toDecimalDigit(const FX_WCHAR
c) {
On 2015/11/03 22:02:51, Tom Sepez wrote:
> If its
>
http://build.chromium.org/p/client.pdfium/builders/windows/builds/56/steps/co...
> 
> The maybe you need a wchar version that calls std::iswxdigit to go along with
> it?

Done.

https://codereview.chromium.org/1433513002/diff/40001/core/include/fxcrt/fx_e...
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1433513002/diff/40001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:40: inline int FXSYS_toHexDigit(const FX_CHAR c) {
Made this FX_CHAR to be consistent.

https://codereview.chromium.org/1433513002/diff/40001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:47: inline int FXSYS_toDecimalDigit(int c) {
Had to change this to int otherwise the compiler couldn't determine if, when
this was called with an int, if it wanted the FX_WCHAR or FX_CHAR versions.

https://codereview.chromium.org/1433513002/diff/40001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/1433513002/diff/40001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc.cpp:15: T FXSYS_StrToInt(const FX_CHAR* str) {
I changed the template as we call std::isdigit or std::iswdigit internally and
couldn't do that when we were templating on FX_CHAR and FX_WCHAR.

[Tom Sepez, 2015-11-04 17:49:25]

I think we're at a point, where much like std::isxdigit() and std::iswxdigit(),
we want to have different names for the char vs. wchar functions, to avoid these
kinds of overload ambiguities. That the std:: folks didn't overload these should
be a strong hint to us to do the same.

[dsinclair, 2015-11-04 17:54:57]

On 2015/11/04 17:49:25, Tom Sepez wrote:
> I think we're at a point, where much like std::isxdigit() and
std::iswxdigit(),
> we want to have different names for the char vs. wchar functions, to avoid
these
> kinds of overload ambiguities. That the std:: folks didn't overload these
should
> be a strong hint to us to do the same.

Better?

[Tom Sepez, 2015-11-04 18:42:46]

Better. But this is a really hard problem to do right.

https://codereview.chromium.org/1433513002/diff/60001/core/include/fxcrt/fx_e...
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1433513002/diff/60001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:40: inline int FXSYS_toHexDigit(const FX_CHAR c) {
Do wen need toHexDigitWide() for parallelism with the below?

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:734: uint8_t digit1 = first.GetAt(i
* 2 + 1);
nit: are these FX_CHARs?

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:10: #include
"../../../include/fxcrt/fx_ext.h"
Do we still need this or did this fix previous IWYU?

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text.cpp:441: if (!std::isdigit(ch) && ch != '-' && ch !=
'+' && ch != '.' && ch != ' ')
this should be iswdigit(), otherwise risk undefined behaviour. It's also nice to
know that "--.+ +.--" is a number.

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

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2436: (char_left > 0xfb00 && char_left <
0xfb06) || std::isdigit(char_left) ||
again, these are wide chars. so std:iswdigit()  Using int for the type instead
of FX_WCHAR is suspicious, too, though all wchars fit in int.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec.cpp:152: #define EXPONENT_DETECT(ptr)           
     \
Later on, you can fix this, too.  I'd minimize it and then just instantiate in
both places, or make it into an inline function. Without actually checking, the
loop should be of the form 
  while (std::isdigit(*ptr)) {
    exp_ret = 10 * exp_ret +  FXSYS_toDecimalDigit(*ptr);
    ++ptr;
  }

or perhaps

  for (exp_ret = 0; std::isdigit(*ptr); ++ptr) ...

also the name is misleading as it doesn't detect exponents, its just anotehr
atoi() equivalent of which you have some elsewhere, right?

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc.cpp:35: template <class T>
If you wanted to get really sneaky, FXSYS_toDecimalDigit itself becomes
templated, but with no generic implementation, just two specializations for
wchar and char.  Then you can template StrToInt on both int type and string
type: 

template <class T, class C>
T FXSYS_StrToInt(const C* str) {...

and invoke FXSYS_toDecimalDigit<C>().  In other places, where it's ambiguous,
you can use <FX_CHAR> to disambiguate.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_util.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:17: #include <cctype>
nit: this goes at line 6.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:110: if (integer < 0)
argh. You can't detect overflow this way. Signed int overflow is undefined
behaviour and the compiler will aggressively remove this check because it knows
that undefined behaviour can't happen.

If we really want this, then we first have to check if integer >
std::numeric_limit<int> / 10 before the multiply and that if integer >
std::numeric_limit<int> - 9 before the add.  There's a way to combine these if
you're clever.

[dsinclair, 2015-11-04 20:16:24]

PTAL. Went through and found a few more places we should be using the Wide
variant. Marked with comments.

https://codereview.chromium.org/1433513002/diff/60001/core/include/fxcrt/fx_e...
File core/include/fxcrt/fx_ext.h (right):

https://codereview.chromium.org/1433513002/diff/60001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:40: inline int FXSYS_toHexDigit(const FX_CHAR c) {
On 2015/11/04 18:42:45, Tom Sepez wrote:
> Do wen need toHexDigitWide() for parallelism with the below?


We don't use it, so I'm skipping it until we need it.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdfapi/fpdf_f...
File core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdfapi/fpdf_f...
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp:734: uint8_t digit1 = first.GetAt(i
* 2 + 1);
On 2015/11/04 18:42:45, Tom Sepez wrote:
> nit: are these FX_CHARs?


first is a CFX_ByteStringC. That class stores the data as uint8_t* and GetAt()
returns a uint8_t. So, in terms of the data we're working with, I think uint8_t
is the correct type.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:10: #include
"../../../include/fxcrt/fx_ext.h"
On 2015/11/04 18:42:45, Tom Sepez wrote:
> Do we still need this or did this fix previous IWYU?

Removed, was vestigial.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text.cpp:441: if (!std::isdigit(ch) && ch != '-' && ch !=
'+' && ch != '.' && ch != ' ')
On 2015/11/04 18:42:45, Tom Sepez wrote:
> this should be iswdigit(), otherwise risk undefined behaviour. It's also nice
to
> know that "--.+ +.--" is a number.

Done. Added todo for --.+ +.--

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

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text_int.cpp:2436: (char_left > 0xfb00 && char_left <
0xfb06) || std::isdigit(char_left) ||
On 2015/11/04 18:42:45, Tom Sepez wrote:
> again, these are wide chars. so std:iswdigit()  Using int for the type instead
> of FX_WCHAR is suspicious, too, though all wchars fit in int.

Done.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec.cpp:152: #define EXPONENT_DETECT(ptr)           
     \
On 2015/11/04 18:42:45, Tom Sepez wrote:
> Later on, you can fix this, too.  I'd minimize it and then just instantiate in
> both places, or make it into an inline function. Without actually checking,
the
> loop should be of the form 
>   while (std::isdigit(*ptr)) {
>     exp_ret = 10 * exp_ret +  FXSYS_toDecimalDigit(*ptr);
>     ++ptr;
>   }
> 
> or perhaps
> 
>   for (exp_ret = 0; std::isdigit(*ptr); ++ptr) ...
> 
> also the name is misleading as it doesn't detect exponents, its just anotehr
> atoi() equivalent of which you have some elsewhere, right?
> 

Acknowledged.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc.cpp:35: template <class T>
On 2015/11/04 18:42:45, Tom Sepez wrote:
> If you wanted to get really sneaky, FXSYS_toDecimalDigit itself becomes
> templated, but with no generic implementation, just two specializations for
> wchar and char.  Then you can template StrToInt on both int type and string
> type: 
> 
> template <class T, class C>
> T FXSYS_StrToInt(const C* str) {...
> 
> and invoke FXSYS_toDecimalDigit<C>().  In other places, where it's ambiguous,
> you can use <FX_CHAR> to disambiguate.


Ack. I'd prefer to stick with dumb, as I'll remember how it works in 2 months...

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_util.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:14: #include <direct.h>
Should these also be moved up?

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:17: #include <cctype>
On 2015/11/04 18:42:46, Tom Sepez wrote:
> nit: this goes at line 6.

Done.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:110: if (integer < 0)
On 2015/11/04 18:42:46, Tom Sepez wrote:
> argh. You can't detect overflow this way. Signed int overflow is undefined
> behaviour and the compiler will aggressively remove this check because it
knows
> that undefined behaviour can't happen.
> 
> If we really want this, then we first have to check if integer >
> std::numeric_limit<int> / 10 before the multiply and that if integer >
> std::numeric_limit<int> - 9 before the add.  There's a way to combine these if
> you're clever.

I'm assuming you meant std::numeric_limits<int>::max() ?


Kept the checks, went with non-clever. I don't know why it checks for overflow
here, but I'm going to assume it's needed until we can prove otherwise. Added a
TODO.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_wstring.cpp:770: while (std::isdigit(*lpsz))
Changed to iswdigit and below.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_wstring.cpp:973: integer = integer * 10 +
FXSYS_toDecimalDigit(str[cc]);
Changed to DigitWide and below.

https://codereview.chromium.org/1433513002/diff/60001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/PublicMethods.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/PublicMethods.cpp:400: nRet = nRet * 10 +
FXSYS_toDecimalDigit(c);
Changed to DigitWide.

[Tom Sepez, 2015-11-04 20:33:11]

Almost.  How about using base's safe numerics instead?  The checks are more
convoluted than I thought.

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdftext/fpdf_...
File core/src/fpdftext/fpdf_text.cpp (right):

https://codereview.chromium.org/1433513002/diff/60001/core/src/fpdftext/fpdf_...
core/src/fpdftext/fpdf_text.cpp:441: if (!std::isdigit(ch) && ch != '-' && ch !=
'+' && ch != '.' && ch != ' ')
On 2015/11/04 20:16:23, dsinclair wrote:
> On 2015/11/04 18:42:45, Tom Sepez wrote:
> > this should be iswdigit(), otherwise risk undefined behaviour. It's also
nice
> to
> > know that "--.+ +.--" is a number.
> 
> Done. Added todo for --.+ +.--
nice.

https://codereview.chromium.org/1433513002/diff/80001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_util.cpp (right):

https://codereview.chromium.org/1433513002/diff/80001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:114: if (integer >
std::numeric_limits<int>::max() - 9)
Sorry, I blew it.  We should be able to parse 4294967295, but now we cant
because at the last character "5", integer is  4294967290 following the
multiplication, and its always larger than 4294967295 - 9.  What we meant to do
is call toDD() earlier

   int digit = FXSYS_toDecimalDigit(str[cc])
then
   if (integer > std::numeric_limits<int>::max() - digit)

[dsinclair, 2015-11-04 21:31:28]

https://codereview.chromium.org/1433513002/diff/80001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_util.cpp (right):

https://codereview.chromium.org/1433513002/diff/80001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:114: if (integer >
std::numeric_limits<int>::max() - 9)
On 2015/11/04 20:33:11, Tom Sepez wrote:
> Sorry, I blew it.  We should be able to parse 4294967295, but now we cant
> because at the last character "5", integer is  4294967290 following the
> multiplication, and its always larger than 4294967295 - 9.  What we meant to
do
> is call toDD() earlier
> 
>    int digit = FXSYS_toDecimalDigit(str[cc])
> then
>    if (integer > std::numeric_limits<int>::max() - digit)


Actually, this code is just broken. If we did overflow, we'd break, and then
take the overflowed value anyway (possibly flipping the sign). All it does when
it overflows is to stop adding new values. It isn't actually handled in any way.

Not sure what the correct result is here, need to check the callers to see what
is done with pData.

[dsinclair, 2015-11-10 00:47:03]

https://codereview.chromium.org/1433513002/diff/80001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_util.cpp (right):

https://codereview.chromium.org/1433513002/diff/80001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_util.cpp:114: if (integer >
std::numeric_limits<int>::max() - 9)
On 2015/11/04 21:31:28, dsinclair wrote:
> On 2015/11/04 20:33:11, Tom Sepez wrote:
> > Sorry, I blew it.  We should be able to parse 4294967295, but now we cant
> > because at the last character "5", integer is  4294967290 following the
> > multiplication, and its always larger than 4294967295 - 9.  What we meant to
> do
> > is call toDD() earlier
> > 
> >    int digit = FXSYS_toDecimalDigit(str[cc])
> > then
> >    if (integer > std::numeric_limits<int>::max() - digit)
> 
> 
> Actually, this code is just broken. If we did overflow, we'd break, and then
> take the overflowed value anyway (possibly flipping the sign). All it does
when
> it overflows is to stop adding new values. It isn't actually handled in any
way.
> 
> Not sure what the correct result is here, need to check the callers to see
what
> is done with pData.


I put it back to the wrong way as that was what the code did before. I left a
todo that it needs to be fixed. This whole function needs to determine what the
right thing to do with overflow is instead of just using the value that
overflowed.

[Tom Sepez, 2015-11-10 17:06:18]

lgtm

[dsinclair, 2015-11-10 18:18:04]

Committed patchset #9 (id:160001) manually as
b27902b8995bb3e003daed6b0811ed746763c68d (presubmit successful).