Fix FXSYS_itoa() implementation.

Fix FXSYS_itoa() implementation.

I thought about removing it, but decided to fix it instead until c++11
hits and there may be better alternatives.

Remove unused variants.

BUG=517854
R=brucedawson@chromium.org, thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/9778206732296e949ef22a79362c6bce34a56052

[Tom Sepez, 2015-08-07 20:12:38]

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

[Tom Sepez, 2015-08-07 20:12:49]

Lei, for review, see bug for details.

[Tom Sepez, 2015-08-07 20:27:17]

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

[Tom Sepez, 2015-08-07 20:27:33]

+Bruce because he knows everything.

[Lei Zhang, 2015-08-07 20:50:12]

https://codereview.chromium.org/1274423003/diff/20001/core/src/fxcrt/fx_syste...
File core/src/fxcrt/fx_system_unittest.cpp (right):

https://codereview.chromium.org/1274423003/diff/20001/core/src/fxcrt/fx_syste...
core/src/fxcrt/fx_system_unittest.cpp:35: buf[kBase16SentinelLocation] =
kSentinel;
Maybe write CheckBase16ItoA(int32_t input, const char* expected_output) rather
than repeating the same 4 lines over and over? Similarly for Base10 and Base2.

https://codereview.chromium.org/1274423003/diff/20001/core/src/fxcrt/fx_syste...
core/src/fxcrt/fx_system_unittest.cpp:49: EXPECT_EQ(kSentinel,
buf[kBase2SentinelLocation]);
Repeat of last line?

[brucedawson, 2015-08-07 20:55:49]

A couple of comments, but lgtm.

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

https://codereview.chromium.org/1274423003/diff/20001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc.cpp:85: const FX_CHAR* FXSYS_i64toa(int64_t value,
FX_CHAR* str, int radix) {
I hate the design of passing in an FX_CHAR* and returning the same one, with no
way of verifying that the buffer is large enough.

If the sizes are standardized in pdfium then this could be improved by changing
the input parameter from "FX_CHAR* str" to "FXCHAR(&str)[34]" (for instance)
which would only compile if a 34 character buffer was passed.

The implementation function could remain as-is, but FXSYS_i64toa would be
changed. Any sites that were passing in a 34 character buffer would handle the
new prototype without needing changing.

Something to consider, for this change or for another.

https://codereview.chromium.org/1274423003/diff/20001/core/src/fxcrt/fx_syste...
File core/src/fxcrt/fx_system_unittest.cpp (right):

https://codereview.chromium.org/1274423003/diff/20001/core/src/fxcrt/fx_syste...
core/src/fxcrt/fx_system_unittest.cpp:15: const size_t kBase2SentinelLocation =
34;  // "-" + 32 digits + null == 34.
I think that the preferred abbreviation for the null character is NUL. This name
helps to distinguish it from NULL (the pointer) or nullptr.

But, although a quick wikipedia search supports this it also suggests that it is
not universal and not important, so feel free to ignore.

[Tom Sepez, 2015-08-07 20:57:31]

> I think that the preferred abbreviation for the null character is NUL. This
name
> helps to distinguish it from NULL (the pointer) or nullptr.
You are right, and that is one of my favorite pet peeves.  Thanks for catching
it.

[Tom Sepez, 2015-08-07 21:58:14]

https://codereview.chromium.org/1274423003/diff/20001/core/src/fxcrt/fx_syste...
File core/src/fxcrt/fx_system_unittest.cpp (right):

https://codereview.chromium.org/1274423003/diff/20001/core/src/fxcrt/fx_syste...
core/src/fxcrt/fx_system_unittest.cpp:35: buf[kBase16SentinelLocation] =
kSentinel;
On 2015/08/07 20:50:12, Lei Zhang wrote:
> Maybe write CheckBase16ItoA(int32_t input, const char* expected_output) rather
> than repeating the same 4 lines over and over? Similarly for Base10 and Base2.

Done.

https://codereview.chromium.org/1274423003/diff/20001/core/src/fxcrt/fx_syste...
core/src/fxcrt/fx_system_unittest.cpp:49: EXPECT_EQ(kSentinel,
buf[kBase2SentinelLocation]);
On 2015/08/07 20:50:12, Lei Zhang wrote:
> Repeat of last line?

Done.

[Lei Zhang, 2015-08-07 22:26:12]

lgtm

[Tom Sepez, 2015-08-07 22:28:26]

Committed patchset #3 (id:40001) manually as
9778206732296e949ef22a79362c6bce34a56052 (presubmit successful).

[Nico, 2015-08-12 16:52:20]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-08-12 16:52:21]

https://codereview.chromium.org/1274423003/diff/40001/core/src/fxcrt/fx_syste...
File core/src/fxcrt/fx_system_unittest.cpp (right):

https://codereview.chromium.org/1274423003/diff/40001/core/src/fxcrt/fx_syste...
core/src/fxcrt/fx_system_unittest.cpp:152: std::numeric_limits<int64_t>::max(),
"7fffffffffffffff");
This new code isn't clang-formatted. Did the presubmit not fire on this?

[Tom Sepez, 2015-08-13 18:16:57]

On 2015/08/12 16:52:21, Nico (hiding) wrote:
>
https://codereview.chromium.org/1274423003/diff/40001/core/src/fxcrt/fx_syste...
> File core/src/fxcrt/fx_system_unittest.cpp (right):
> 
>
https://codereview.chromium.org/1274423003/diff/40001/core/src/fxcrt/fx_syste...
> core/src/fxcrt/fx_system_unittest.cpp:152:
std::numeric_limits<int64_t>::max(),
> "7fffffffffffffff");
> This new code isn't clang-formatted. Did the presubmit not fire on this?
No presubmit, This was landed from a standalone checkout -- have you tried both
chromium and standalone pdfium checkouts to ensure that it works in both cases?

[Lei Zhang, 2015-08-13 18:18:10]

On 2015/08/12 16:52:21, Nico (hiding) wrote:
> This new code isn't clang-formatted. Did the presubmit not fire on this?

The presubmit has been firing for my CLs.