Update safe numerics package to get bitwise ops

Update safe numerics package to get bitwise ops

Fix callers conventions to avoid ambiguity.
Fix bad bounds check unmasked by change.
Directly include headers no longer pulled in by numerics itself.

Review-Url: https://codereview.chromium.org/2640143003
Committed: https://pdfium.googlesource.com/pdfium/+/4022f87eb8716155291543efaaf289e51d7cbf43

[Tom Sepez, 2017-01-19 19:48:26]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 19:48:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2017-01-19 19:51:10]

Description was changed from

==========
Update safe numerics package to get bitwise ops

Fix callers conventions to avoid ambiguity.
Directly include headers no longer pulled in by numerics itself.
==========

to

==========
Update safe numerics package to get bitwise ops

Fix callers conventions to avoid ambiguity.
Directly include headers no longer pulled in by numerics itself.
==========

[Tom Sepez, 2017-01-19 19:51:10]

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

[Tom Sepez, 2017-01-19 19:51:19]

Justin, ptal.

[Tom Sepez, 2017-01-19 19:52:48]

https://codereview.chromium.org/2640143003/diff/20001/core/fxge/ge/fx_ge_text...
File core/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/2640143003/diff/20001/core/fxge/ge/fx_ge_text...
core/fxge/ge/fx_ge_text.cpp:83: std::min(rect.left,
static_cast<int>(char_left.ValueOrDie<int>()));
Cleaner way to do this?  std::min() and std::max() are trouble.

[commit-bot: I haz the power, 2017-01-19 19:54:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 19:54:21]

Dry run: Try jobs failed on following builders:
  mac on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac/builds/3397)

[Tom Sepez, 2017-01-19 20:51:51]

Description was changed from

==========
Update safe numerics package to get bitwise ops

Fix callers conventions to avoid ambiguity.
Directly include headers no longer pulled in by numerics itself.
==========

to

==========
Update safe numerics package to get bitwise ops

Fix callers conventions to avoid ambiguity.
Fix bad bounds check unmasked by change.
Directly include headers no longer pulled in by numerics itself.
==========

[Tom Sepez, 2017-01-19 20:52:03]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 20:52:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 20:57:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 20:57:34]

Dry run: Try jobs failed on following builders:
  mac_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_xfa_rel/bui...)

[Tom Sepez, 2017-01-19 21:07:37]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 21:07:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2017-01-20 17:32:35]

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

[dsinclair, 2017-01-23 14:46:33]

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/font/fpdf_...
File core/fpdfapi/font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/font/fpdf_...
core/fpdfapi/font/fpdf_font_cid.cpp:21: #include "third_party/base/logging.h"
What is this for?

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_indirect_object_holder.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_indirect_object_holder.cpp:14: #include
"third_party/base/logging.h"
?

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/cfx_render...
File core/fxge/ge/cfx_renderdevice.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/cfx_render...
core/fxge/ge/cfx_renderdevice.cpp:1025: int start_col =
std::max(static_cast<int>(left.ValueOrDie<int>()), 0);
If this is templated as <int> why do we need the cast?

[jschuh, 2017-01-23 17:05:19]

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/font/fpdf_...
File core/fpdfapi/font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/font/fpdf_...
core/fpdfapi/font/fpdf_font_cid.cpp:21: #include "third_party/base/logging.h"
On 2017/01/23 14:46:33, dsinclair wrote:
> What is this for?

I removed all dependencies on base/logging.h from base/numerics. Looks like this
fixes an IWYU bug by explicitly adding that back in.

https://codereview.chromium.org/2640143003/diff/80001/core/fxcrt/fx_basic_arr...
File core/fxcrt/fx_basic_array.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fxcrt/fx_basic_arr...
core/fxcrt/fx_basic_array.cpp:36: m_pData = FX_Alloc(uint8_t,
totalSize.ValueOrDie<size_t>());
Weird. I'm not sure how this compiles without doing one of these (equivalent)
things:
  totalSize.template ValueOrDie<size_t>()
          - or -
  ValueOrDieForType<size_t>(totalSize)

https://codereview.chromium.org/2640143003/diff/80001/core/fxcrt/fx_basic_arr...
core/fxcrt/fx_basic_array.cpp:52: FX_Realloc(uint8_t, m_pData,
totalSize.ValueOrDie<size_t>());
Same question for this and the other uses below.

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/cfx_render...
File core/fxge/ge/cfx_renderdevice.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/cfx_render...
core/fxge/ge/cfx_renderdevice.cpp:1025: int start_col =
std::max(static_cast<int>(left.ValueOrDie<int>()), 0);
On 2017/01/23 14:46:33, dsinclair wrote:
> If this is templated as <int> why do we need the cast?

Because the type returned from ValueOrDie is a StrictNumeric, which will throw a
compile error because the template provides too many valid conversions. The
better solution is to just use this variadic template function:

  int start_col = base::CheckMax(left, 0).template ValueOrDie<int>();

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/fx_ge_text...
File core/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/fx_ge_text...
core/fxge/ge/fx_ge_text.cpp:83: std::min(rect.left,
static_cast<int>(char_left.ValueOrDie<int>()));
Same as above. This is what CheckMax() is for.

[Tom Sepez, 2017-01-23 18:19:14]

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/font/fpdf_...
File core/fpdfapi/font/fpdf_font_cid.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/font/fpdf_...
core/fpdfapi/font/fpdf_font_cid.cpp:21: #include "third_party/base/logging.h"
On 2017/01/23 17:05:19, jschuh wrote:
> On 2017/01/23 14:46:33, dsinclair wrote:
> > What is this for?
> 
> I removed all dependencies on base/logging.h from base/numerics. Looks like
this
> fixes an IWYU bug by explicitly adding that back in.

Exactly, there's an invocation of NOTREACHED() at line 191, for instance.

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_indirect_object_holder.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_indirect_object_holder.cpp:14: #include
"third_party/base/logging.h"
On 2017/01/23 14:46:33, dsinclair wrote:
> ?

IWYU for invocation of CHECK() at line 56

https://codereview.chromium.org/2640143003/diff/80001/core/fxcrt/fx_basic_arr...
File core/fxcrt/fx_basic_array.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fxcrt/fx_basic_arr...
core/fxcrt/fx_basic_array.cpp:36: m_pData = FX_Alloc(uint8_t,
totalSize.ValueOrDie<size_t>());
On 2017/01/23 17:05:19, jschuh wrote:
> Weird. I'm not sure how this compiles without doing one of these (equivalent)
> things:
>   totalSize.template ValueOrDie<size_t>()
>           - or -
>   ValueOrDieForType<size_t>(totalSize)
Ok, I'll switch to the functional form just in case we get a less sophisticated
compiler front end sometime down the road ...

https://codereview.chromium.org/2640143003/diff/80001/core/fxcrt/fx_basic_arr...
core/fxcrt/fx_basic_array.cpp:52: FX_Realloc(uint8_t, m_pData,
totalSize.ValueOrDie<size_t>());
On 2017/01/23 17:05:19, jschuh wrote:
> Same question for this and the other uses below.

Done.

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/cfx_render...
File core/fxge/ge/cfx_renderdevice.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/cfx_render...
core/fxge/ge/cfx_renderdevice.cpp:1025: int start_col =
std::max(static_cast<int>(left.ValueOrDie<int>()), 0);
On 2017/01/23 17:05:19, jschuh wrote:
> On 2017/01/23 14:46:33, dsinclair wrote:
> > If this is templated as <int> why do we need the cast?
> 
> Because the type returned from ValueOrDie is a StrictNumeric, which will throw
a
> compile error because the template provides too many valid conversions. The
> better solution is to just use this variadic template function:
> 
>   int start_col = base::CheckMax(left, 0).template ValueOrDie<int>();
done, used functional form rather than template keyword.

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/fx_ge_text...
File core/fxge/ge/fx_ge_text.cpp (right):

https://codereview.chromium.org/2640143003/diff/80001/core/fxge/ge/fx_ge_text...
core/fxge/ge/fx_ge_text.cpp:83: std::min(rect.left,
static_cast<int>(char_left.ValueOrDie<int>()));
On 2017/01/23 17:05:19, jschuh wrote:
> Same as above. This is what CheckMax() is for.

Done.

[Tom Sepez, 2017-01-23 19:17:31]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 19:17:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 21:02:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 21:02:12]

Dry run: This issue passed the CQ dry run.

[jschuh, 2017-01-23 22:30:17]

lgtm

[Tom Sepez, 2017-01-23 22:35:53]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2017-01-23 22:36:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 22:36:21]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485210953700570,
"parent_rev": "b76f49b36baffea2b2ecb90d67c7b6bb734e7bb9", "commit_rev":
"4022f87eb8716155291543efaaf289e51d7cbf43"}

[commit-bot: I haz the power, 2017-01-23 22:36:24]

Description was changed from

==========
Update safe numerics package to get bitwise ops

Fix callers conventions to avoid ambiguity.
Fix bad bounds check unmasked by change.
Directly include headers no longer pulled in by numerics itself.
==========

to

==========
Update safe numerics package to get bitwise ops

Fix callers conventions to avoid ambiguity.
Fix bad bounds check unmasked by change.
Directly include headers no longer pulled in by numerics itself.

Review-Url: https://codereview.chromium.org/2640143003
Committed:
https://pdfium.googlesource.com/pdfium/+/4022f87eb8716155291543efaaf289e51d7c...
==========

[commit-bot: I haz the power, 2017-01-23 22:36:25]

Committed patchset #6 (id:100001) as
https://pdfium.googlesource.com/pdfium/+/4022f87eb8716155291543efaaf289e51d7c...