Re-enable MSVC warning 4800 for compiling with chromium_code

Re-enable MSVC warning 4800 for compiling with chromium_code

Mainly change the code to avoid the warnings; in a few cases we have
to use explicit casts.

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

Committed: https://pdfium.googlesource.com/pdfium/+/97da97662417085774f75c26e535c6fbe70266ae

[Wei Li, 2016-03-10 23:03:46]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-03-10 23:15:24]

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

[Wei Li, 2016-03-10 23:15:25]

I think GetSimilarValue() in fx_ge_fontmap.cpp had a bug on the logic. The
change should be the intended behavior. 

PTAL, thanks!

[Tom Sepez, 2016-03-10 23:44:34]

On 2016/03/10 23:15:25, Wei Li wrote:
> I think GetSimilarValue() in fx_ge_fontmap.cpp had a bug on the logic. The
> change should be the intended behavior. 
> 
> PTAL, thanks!

Can we change the CL description to "Re-enable MSVC warning 4800" ? I wasn't
sure at first glance whether "removing" meant suppression or non-suppression. 
I'm looking at the rest now.

[Wei Li, 2016-03-10 23:55:32]

Description was changed from

==========
Remove all msvc 4800 warnings

Mainly change the code to avoid the warnings; in a few cases we have
to use explicit casts.

BUG=pdfium:29
==========

to

==========
Re-enable MSVC warning 4800 for chromium_code

Mainly change the code to avoid the warnings; in a few cases we have
to use explicit casts.

BUG=pdfium:29
==========

[Wei Li, 2016-03-10 23:56:14]

Description was changed from

==========
Re-enable MSVC warning 4800 for chromium_code

Mainly change the code to avoid the warnings; in a few cases we have
to use explicit casts.

BUG=pdfium:29
==========

to

==========
Re-enable MSVC warning 4800 for compiling with chromium_code

Mainly change the code to avoid the warnings; in a few cases we have
to use explicit casts.

BUG=pdfium:29
==========

[Wei Li, 2016-03-10 23:56:39]

On 2016/03/10 23:44:34, Tom Sepez wrote:
> On 2016/03/10 23:15:25, Wei Li wrote:
> > I think GetSimilarValue() in fx_ge_fontmap.cpp had a bug on the logic. The
> > change should be the intended behavior. 
> > 
> > PTAL, thanks!
> 
> Can we change the CL description to "Re-enable MSVC warning 4800" ? I wasn't
> sure at first glance whether "removing" meant suppression or non-suppression. 
> I'm looking at the rest now.

Done

[Tom Sepez, 2016-03-11 00:30:29]

https://codereview.chromium.org/1783023002/diff/20001/core/include/fpdfapi/cp...
File core/include/fpdfapi/cpdf_parser.h (right):

https://codereview.chromium.org/1783023002/diff/20001/core/include/fpdfapi/cp...
core/include/fpdfapi/cpdf_parser.h:133: bool m_bVersionUpdated;
Poking around for how this is used, I noticed this was uninitialized by the
ctor.  we should probably set it to false there.

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

https://codereview.chromium.org/1783023002/diff/20001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:63: return std::isdigit(c) != 0;
nit: prefer !!std::isdigit(c) 
also below.

https://codereview.chromium.org/1783023002/diff/20001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:71: if (!std::isdigit(c))
nit: as long as we're here, how about
return std::isdigit(c) ? c - '0' : 0;

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp:270: for (size_t i = 0; i <
dwSharedObjTotal; ++i) {
nit: how about FX_DWORD, to match the type of dwSharedObjTotal?

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/cpdf_standard_security_handler.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/cpdf_standard_security_handler.cpp:386: if
(FXDWORD_GET_LSBFIRST(buf) != static_cast<int>(m_Permissions)) {
I'd really  like the return vale of FXDWORD_GET_LSBFIRST to be a FX_DWORD.  Its
macro is defined as 

#define FXDWORD_GET_LSBFIRST(p) \
  ((p[3] << 24) | (p[2] << 16) | (p[1] << 8) | (p[0]))

where p is expected to be an uint8_t, which when shifted gets promoted to int,
(not unsigned) because uint8_t fits in an int.

what we want is (((FX_DWORD)p[3]) << 24) | ... since left shifting a dword gets
promoted to unsigned int since dword doesn't fit in an int.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:631: int comps, bpc;
also one declaration per line while we're at it.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1443: FX_DWORD last_src_x
= src_width;
Ok, took a moment for me to get this, maybe a comment here that src_x will
always be less than src_width so the first comparison will always fail.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formfield.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formfield.cpp:121: CheckControl(i, bChecked, FALSE);
or just CheckControl(i, pControl->IsDefaultChecked(), FALSE);

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

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_wstring.cpp:208: return wcslen(ptr) ==
static_cast<unsigned int>(m_pData->m_nDataLength) &&
wcslen() returns a size_t, so maybe we want to cast to that.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
File core/src/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:390: if (((style & FXFONT_BOLD) != 0) ==
(weight > 400)) {
again, !!

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:397: ((pitch_family & FXFONT_FF_ROMAN) != 0))
{
Ooops. FXFONT_FF_ROMAN  is (1 << 4), and FXFONT_SERIF is 0x02, so this
expression can never be true as originally written.  Hmmm.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:400: if (((style & FXFONT_SCRIPT) != 0) ==
FXFONT_SCRIPT is 8, FXFONT_FF_SCRIPT is (4 << 4).  Ugh.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:404: if (((style & FXFONT_FIXED_PITCH) != 0)
==
check this too.

Anyway, I think your changes more accurately express their intent.

https://codereview.chromium.org/1783023002/diff/20001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/Document.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/Document.cpp:806: pValueObj->GetInteger() != 0);
nit: !!

https://codereview.chromium.org/1783023002/diff/20001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/Field.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/Field.cpp:3042: pFormField->CheckControl(nWidget,
bCheckit, true);
Same on either branch?

[Wei Li, 2016-03-11 04:11:37]

Thanks for the review!

https://codereview.chromium.org/1783023002/diff/20001/core/include/fpdfapi/cp...
File core/include/fpdfapi/cpdf_parser.h (right):

https://codereview.chromium.org/1783023002/diff/20001/core/include/fpdfapi/cp...
core/include/fpdfapi/cpdf_parser.h:133: bool m_bVersionUpdated;
On 2016/03/11 00:30:28, Tom Sepez wrote:
> Poking around for how this is used, I noticed this was uninitialized by the
> ctor.  we should probably set it to false there.

Done.

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

https://codereview.chromium.org/1783023002/diff/20001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:63: return std::isdigit(c) != 0;
On 2016/03/11 00:30:28, Tom Sepez wrote:
> nit: prefer !!std::isdigit(c) 
> also below.

Done.

https://codereview.chromium.org/1783023002/diff/20001/core/include/fxcrt/fx_e...
core/include/fxcrt/fx_ext.h:71: if (!std::isdigit(c))
On 2016/03/11 00:30:28, Tom Sepez wrote:
> nit: as long as we're here, how about
> return std::isdigit(c) ? c - '0' : 0;

Done.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp:270: for (size_t i = 0; i <
dwSharedObjTotal; ++i) {
On 2016/03/11 00:30:28, Tom Sepez wrote:
> nit: how about FX_DWORD, to match the type of dwSharedObjTotal?

Done.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/cpdf_standard_security_handler.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/cpdf_standard_security_handler.cpp:386: if
(FXDWORD_GET_LSBFIRST(buf) != static_cast<int>(m_Permissions)) {
On 2016/03/11 00:30:28, Tom Sepez wrote:
> I'd really  like the return vale of FXDWORD_GET_LSBFIRST to be a FX_DWORD. 
Its
> macro is defined as 
> 
> #define FXDWORD_GET_LSBFIRST(p) \
>   ((p[3] << 24) | (p[2] << 16) | (p[1] << 8) | (p[0]))
> 
> where p is expected to be an uint8_t, which when shifted gets promoted to int,
> (not unsigned) because uint8_t fits in an int.
> 
> what we want is (((FX_DWORD)p[3]) << 24) | ... since left shifting a dword
gets
> promoted to unsigned int since dword doesn't fit in an int.

Done.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_r...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:631: int comps, bpc;
On 2016/03/11 00:30:28, Tom Sepez wrote:
> also one declaration per line while we're at it.

Done.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfapi/fpdf_r...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1443: FX_DWORD last_src_x
= src_width;
On 2016/03/11 00:30:28, Tom Sepez wrote:
> Ok, took a moment for me to get this, maybe a comment here that src_x will
> always be less than src_width so the first comparison will always fail.

Done.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfdoc/doc_fo...
File core/src/fpdfdoc/doc_formfield.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fpdfdoc/doc_fo...
core/src/fpdfdoc/doc_formfield.cpp:121: CheckControl(i, bChecked, FALSE);
On 2016/03/11 00:30:28, Tom Sepez wrote:
> or just CheckControl(i, pControl->IsDefaultChecked(), FALSE);

Done.

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

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_wstring.cpp:208: return wcslen(ptr) ==
static_cast<unsigned int>(m_pData->m_nDataLength) &&
On 2016/03/11 00:30:28, Tom Sepez wrote:
> wcslen() returns a size_t, so maybe we want to cast to that.

Done.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
File core/src/fxge/ge/fx_ge_fontmap.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:390: if (((style & FXFONT_BOLD) != 0) ==
(weight > 400)) {
On 2016/03/11 00:30:29, Tom Sepez wrote:
> again, !!
> 

Done.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:397: ((pitch_family & FXFONT_FF_ROMAN) != 0))
{
On 2016/03/11 00:30:29, Tom Sepez wrote:
> Ooops. FXFONT_FF_ROMAN  is (1 << 4), and FXFONT_SERIF is 0x02, so this
> expression can never be true as originally written.  Hmmm.

Acknowledged.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:400: if (((style & FXFONT_SCRIPT) != 0) ==
On 2016/03/11 00:30:29, Tom Sepez wrote:
> FXFONT_SCRIPT is 8, FXFONT_FF_SCRIPT is (4 << 4).  Ugh.

Acknowledged.

https://codereview.chromium.org/1783023002/diff/20001/core/src/fxge/ge/fx_ge_...
core/src/fxge/ge/fx_ge_fontmap.cpp:404: if (((style & FXFONT_FIXED_PITCH) != 0)
==
On 2016/03/11 00:30:29, Tom Sepez wrote:
> check this too.
> 
> Anyway, I think your changes more accurately express their intent.

Acknowledged.

https://codereview.chromium.org/1783023002/diff/20001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/Document.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/Document.cpp:806: pValueObj->GetInteger() != 0);
On 2016/03/11 00:30:29, Tom Sepez wrote:
> nit: !!

Done.

https://codereview.chromium.org/1783023002/diff/20001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/Field.cpp (right):

https://codereview.chromium.org/1783023002/diff/20001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/Field.cpp:3042: pFormField->CheckControl(nWidget,
bCheckit, true);
On 2016/03/11 00:30:29, Tom Sepez wrote:
> Same on either branch?

A couple places like this. Add TODOs to check whether they are intentional.

[Tom Sepez, 2016-03-11 20:21:46]

Great. Thanks. LGTM.

[Wei Li, 2016-03-12 01:01:02]

Description was changed from

==========
Re-enable MSVC warning 4800 for compiling with chromium_code

Mainly change the code to avoid the warnings; in a few cases we have
to use explicit casts.

BUG=pdfium:29
==========

to

==========
Re-enable MSVC warning 4800 for compiling with chromium_code

Mainly change the code to avoid the warnings; in a few cases we have
to use explicit casts.

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

Committed:
https://pdfium.googlesource.com/pdfium/+/97da97662417085774f75c26e535c6fbe702...
==========

[Wei Li, 2016-03-12 01:01:04]

Committed patchset #3 (id:60001) manually as
97da97662417085774f75c26e535c6fbe70266ae (presubmit successful).