Re-enable several MSVC warnings

Re-enable several MSVC warnings

Re-enable the following warnings:
 4245: signed/unsigned conversion mismatch;
 4310: cast may truncate data;
 4389: operator on signed/unsigned mismatch;
 4701: use potentially uninitialized local variable;
 4706: assignment within conditional expression

Clean up the code to avoid those warnings.

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

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

[Wei Li, 2016-03-15 23:31:46]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-03-15 23:45:42]

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

[Wei Li, 2016-03-15 23:45:43]

PTAL, thanks

[dsinclair, 2016-03-16 13:23:30]

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

[dsinclair, 2016-03-16 13:23:32]

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp
File core/fpdfdoc/doc_ap.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp...
core/fpdfdoc/doc_ap.cpp:307: if (pDRDict && (pDRFontDict =
pDRDict->GetDictBy("Font")) != nullptr) {
What was the issue with this one?

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_jpx_opj.cpp:32: return static_cast<OPJ_SIZE_T>(-1);
Can you add a TODO, this seems broken. OPJ_SIZE is a typedef for size_t. size_t
is a unsigned data type. We probably shouldn't be using -1 as a sentinal value
in that case.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_tiff.cpp:125: return static_cast<toff_t>(-1);
ditto, toff_t is a uint64.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_Image.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Image.cpp:668: int32_t xs0 = 0, ys0 = 0, xs1 = 0, ys1 =
0;
nit: one per line.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_TrdProc.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_TrdProc.cpp:24: int32_t RDWI = 0, RDHI = 0, RDXI = 0,
RDYI = 0;
ditto

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_TrdProc.cpp:44: FX_BOOL bFirst = TRUE;
nit: bool

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
File core/fxge/dib/fx_dib_main.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
core/fxge/dib/fx_dib_main.cpp:1244: int fc = 0, fm = 0, fy = 0, fk = 0;
nit: one per line

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

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:525: return static_cast<FX_DWORD>(-1);
Ditto: FX_DWORD is a uint64

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

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/javascript/Docu...
fpdfsdk/javascript/Document.cpp:1462: FX_DWORD charcode =
static_cast<FX_DWORD>(-1);
ditto

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/javascript/Docu...
fpdfsdk/javascript/Document.cpp:1495: FX_DWORD charcode =
static_cast<FX_DWORD>(-1);
ditto

[Wei Li, 2016-03-16 17:51:32]

thanks

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp
File core/fpdfdoc/doc_ap.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp...
core/fpdfdoc/doc_ap.cpp:307: if (pDRDict && (pDRFontDict =
pDRDict->GetDictBy("Font")) != nullptr) {
On 2016/03/16 13:23:31, dsinclair wrote:
> What was the issue with this one?

4706, assignment within conditional expression.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_jpx_opj.cpp:32: return static_cast<OPJ_SIZE_T>(-1);
On 2016/03/16 13:23:31, dsinclair wrote:
> Can you add a TODO, this seems broken. OPJ_SIZE is a typedef for size_t.
size_t
> is a unsigned data type. We probably shouldn't be using -1 as a sentinal value
> in that case.

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_tiff.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_tiff.cpp:125: return static_cast<toff_t>(-1);
On 2016/03/16 13:23:31, dsinclair wrote:
> ditto, toff_t is a uint64.

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_Image.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Image.cpp:668: int32_t xs0 = 0, ys0 = 0, xs1 = 0, ys1 =
0;
On 2016/03/16 13:23:31, dsinclair wrote:
> nit: one per line.

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_TrdProc.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_TrdProc.cpp:24: int32_t RDWI = 0, RDHI = 0, RDXI = 0,
RDYI = 0;
On 2016/03/16 13:23:32, dsinclair wrote:
> ditto

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_TrdProc.cpp:44: FX_BOOL bFirst = TRUE;
On 2016/03/16 13:23:32, dsinclair wrote:
> nit: bool

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
File core/fxge/dib/fx_dib_main.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
core/fxge/dib/fx_dib_main.cpp:1244: int fc = 0, fm = 0, fy = 0, fk = 0;
On 2016/03/16 13:23:32, dsinclair wrote:
> nit: one per line

Done.

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

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:525: return static_cast<FX_DWORD>(-1);
On 2016/03/16 13:23:32, dsinclair wrote:
> Ditto: FX_DWORD is a uint64

Done.

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

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/javascript/Docu...
fpdfsdk/javascript/Document.cpp:1462: FX_DWORD charcode =
static_cast<FX_DWORD>(-1);
On 2016/03/16 13:23:32, dsinclair wrote:
> ditto

Done.

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/javascript/Docu...
fpdfsdk/javascript/Document.cpp:1495: FX_DWORD charcode =
static_cast<FX_DWORD>(-1);
On 2016/03/16 13:23:32, dsinclair wrote:
> ditto

Done.

[Tom Sepez, 2016-03-16 18:35:00]

Can you split out all the ones that I raised big questions on to another CL? 
That way we can land the simple stuff first.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_edit/...
File core/fpdfapi/fpdf_edit/fpdf_edit_doc.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_edit/...
core/fpdfapi/fpdf_edit/fpdf_edit_doc.cpp:939: ordering = "GB1", supplement = 2;
nit: put on own line, I missed this the first time reading this.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page.cpp:716: int x0 = 0;
I suspect this is only required because iRotate is signed and the compiler
worries that iRotate % 4 could be less than 0, and not covered in the switch. 
If we made this an unsigned type, can we avoid the initialiations here? how far
up does that ripple?

If we don't have to intialize here, we'd catch the case when someone
accidentally deletes one of the assignments in the switch.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:44: return (FX_DWORD)-1;
nit:static_cast

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
core/fpdfapi/fpdf_render/fpdf_render_text.cpp:140: if
(FXSYS_fabs(image_matrix.b) < FXSYS_fabs(image_matrix.a) / 100 &&
Do we know if this is ever false? If we could somehow remove it, the
initializations wouldn't be required.  I can't see how it works otherwise.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
core/fpdfapi/fpdf_render/fpdf_render_text.cpp:142: int top_line, bottom_line;
nit: one per line, combine with assignments.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
core/fpdfapi/fpdf_render/fpdf_render_text.cpp:166: } else {
nit: empty else.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp
File core/fpdfdoc/doc_ap.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp...
core/fpdfdoc/doc_ap.cpp:306: CPDF_Dictionary* pDRFontDict = NULL;
how about:
CPDF_Dictionary* pDRFontDict = pDRDict ? pDRDict->GetDictBy("Font") : nullptr;
if (pDRFontDict) {

https://codereview.chromium.org/1801383002/diff/20001/core/fpdftext/fpdf_text...
File core/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:1806: CPDF_TextObjectItem itemPer = {0, 0.0f,
0.0f};
We may want to ping the fx folks here.  Did the intend to make the check at line
1815 with every iteration, or just on the last character? Otherwise, can we add
an early return of true here if nPreCount == 0, and then let the compiler prove
that the loop must be executed?

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_jpx_opj.cpp:32: return static_cast<OPJ_SIZE_T>(-1);
On 2016/03/16 13:23:31, dsinclair wrote:
> Can you add a TODO, this seems broken. OPJ_SIZE is a typedef for size_t.
size_t
> is a unsigned data type. We probably shouldn't be using -1 as a sentinal value
> in that case.

Probably ok along the lines of std::string::npos's usage of the same technique.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Context.cpp:205: if (m_PauseStep <= 1) {
nit: pity m_PauseStep isn't an enum with a switch here so the compiler could
prove all cases were covered.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Context.cpp:686: uint8_t cSBHUFFFS = 0;
yup.  Good catch.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_Image.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Image.cpp:668: int32_t xs0 = 0, ys0 = 0, xs1 = 0, ys1 =
0;
I didn't see how these could be used uninitialized.  Also, can we move these
down to right before they are used? eg. declare ys0 at 683?

These might clean up if we combined declaration and initialization using the ?
operator.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Image.cpp:678: if (x < -1048576 || x > 1048576 || y <
-1048576 || y > 1048576) {
nit: they probably mean >= 1048576, since x wont fit in 20 bits if it is
1048576.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Image.cpp:824: int32_t middleDwords = 0;
I don't think this needs to persist between blocks, since we initialize it each
time.  Maybe use N decls of this at line 828, 920, etc.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_TrdProc.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_TrdProc.cpp:26: int32_t nVal = 0, nBits = 0;
These are probably OK since the compiler can't prove that a value will be set
when passing these by &STRIPT to decodeAsValue().  

This would benefit from declaring these right before they get clobbered by the
decode calls.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_map...
File core/fxcrt/fx_basic_maps.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_map...
core/fxcrt/fx_basic_maps.cpp:42: if ((pAssocRet = m_pHashTable[nBucket]) !=
nullptr)
what's the warning here?

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_map...
core/fxcrt/fx_basic_maps.cpp:48: if ((pAssocNext = pAssocRet->pNext) == NULL) {
nit: move assignment to line 47/

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_map...
core/fxcrt/fx_basic_maps.cpp:50: nBucket < m_nHashTableSize; nBucket++) {
I think if you add && !pAssocNext to the loop condition, you can lose the if at
line 48 and write line 51 - 53 as just
   pAssocNext = m_pHashTable[nBucket];

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_wst...
File core/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_wst...
core/fxcrt/fx_basic_wstring.cpp:644: {
nit: brace needed here?

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_wst...
core/fxcrt/fx_basic_wstring.cpp:647: lpszStart < lpszEnd) {
This should be fine.  Not sure we want to write it this way to make the compiler
happy.

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_co...
File core/fxge/dib/fx_dib_composite.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_co...
core/fxge/dib/fx_dib_composite.cpp:212: switch (blend_mode) {
nit: again, if this were an enum, the compiler might be able to prove that we
cover all cases.

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
File core/fxge/dib/fx_dib_main.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
core/fxge/dib/fx_dib_main.cpp:1239: ASSERT(!IsAlphaMask());
We can lose the assert here since we handle it at 1240

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
core/fxge/dib/fx_dib_main.cpp:1245: int bc = 0, bm = 0, by = 0, bk = 0;
Stunning lack of duplication between the isCmykImage and !isCmykImage paths. 
I'd suggest splitting this into two helper functions.

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

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:525: return static_cast<FX_DWORD>(-1);
On 2016/03/16 17:51:31, Wei Li wrote:
> On 2016/03/16 13:23:32, dsinclair wrote:
> > Ditto: FX_DWORD is a uint64
> 
> Done.
Nah, s'awwright.

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/win32/fx_win3...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/win32/fx_win3...
core/fxge/win32/fx_win32_gdipext.cpp:1004: switch (pGraphState->m_LineJoin) {
again, using an enum might prove coverage.

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fpdfppo.cpp
File fpdfsdk/fpdfppo.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fpdfppo.cpp#new...
fpdfsdk/fpdfppo.cpp:88: FX_DWORD Kidsobjnum =
pDestPDFDoc->AddIndirectObject(pNewKids);
no kidding. !!! :)

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fpdfview.cpp
File fpdfsdk/fpdfview.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fpdfview.cpp#ne...
fpdfsdk/fpdfview.cpp:286: switch (err) {
Strage, we're not missing coverage for one of these.  And it still complains?

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fxedit/fxet_ap.cpp
File fpdfsdk/fxedit/fxet_ap.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fxedit/fxet_ap....
fpdfsdk/fxedit/fxet_ap.cpp:21: FX_DWORD dwCharCode = 0;
Init shouldn't be required given lines 23-26

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fxedit/fxet_ap....
fpdfsdk/fxedit/fxet_ap.cpp:22: 
nit: no blank line here.

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

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/javascript/Docu...
fpdfsdk/javascript/Document.cpp:1462: FX_DWORD charcode =
static_cast<FX_DWORD>(-1);
On 2016/03/16 17:51:32, Wei Li wrote:
> On 2016/03/16 13:23:32, dsinclair wrote:
> > ditto
> 
> Done.

meh.

[Tom Sepez, 2016-03-16 18:37:50]

Also, we may want to live with 4706, doing if ((x = foo)) {} gets caught by
gcc/clang unless it is double-paren'd.  That should be good enough.

[Wei Li, 2016-03-17 00:14:07]

Patchset #3 (id:60001) has been deleted

[Wei Li, 2016-03-17 01:13:59]

Patchset #3 (id:80001) has been deleted

[Wei Li, 2016-03-17 02:24:06]

Thank you for the review.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_edit/...
File core/fpdfapi/fpdf_edit/fpdf_edit_doc.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_edit/...
core/fpdfapi/fpdf_edit/fpdf_edit_doc.cpp:939: ordering = "GB1", supplement = 2;
On 2016/03/16 18:34:58, Tom Sepez wrote:
> nit: put on own line, I missed this the first time reading this.

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page.cpp:716: int x0 = 0;
On 2016/03/16 18:34:58, Tom Sepez wrote:
> I suspect this is only required because iRotate is signed and the compiler
> worries that iRotate % 4 could be less than 0, and not covered in the switch. 
> If we made this an unsigned type, can we avoid the initialiations here? how
far
> up does that ripple?
> 
> If we don't have to intialize here, we'd catch the case when someone
> accidentally deletes one of the assignments in the switch.

I wish the compiler could be smarter. Unfortunately, it doesn't seem doing more
static analysis in this case. Changing to unsigned, or even enum won't help.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:44: return (FX_DWORD)-1;
On 2016/03/16 18:34:58, Tom Sepez wrote:
> nit:static_cast

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
core/fpdfapi/fpdf_render/fpdf_render_text.cpp:140: if
(FXSYS_fabs(image_matrix.b) < FXSYS_fabs(image_matrix.a) / 100 &&
On 2016/03/16 18:34:58, Tom Sepez wrote:
> Do we know if this is ever false? If we could somehow remove it, the
> initializations wouldn't be required.  I can't see how it works otherwise.

Yes, it could be true or false. Tricky thing here is that |left| is passed by
reference as an output argument in line 170.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
core/fpdfapi/fpdf_render/fpdf_render_text.cpp:142: int top_line, bottom_line;
On 2016/03/16 18:34:58, Tom Sepez wrote:
> nit: one per line, combine with assignments.

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
core/fpdfapi/fpdf_render/fpdf_render_text.cpp:166: } else {
On 2016/03/16 18:34:58, Tom Sepez wrote:
> nit: empty else.

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp
File core/fpdfdoc/doc_ap.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp...
core/fpdfdoc/doc_ap.cpp:306: CPDF_Dictionary* pDRFontDict = NULL;
On 2016/03/16 18:34:58, Tom Sepez wrote:
> how about:
> CPDF_Dictionary* pDRFontDict = pDRDict ? pDRDict->GetDictBy("Font") : nullptr;
> if (pDRFontDict) {

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdftext/fpdf_text...
File core/fpdftext/fpdf_text_int.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdftext/fpdf_text...
core/fpdftext/fpdf_text_int.cpp:1806: CPDF_TextObjectItem itemPer = {0, 0.0f,
0.0f};
On 2016/03/16 18:34:58, Tom Sepez wrote:
> We may want to ping the fx folks here.  Did the intend to make the check at
line
> 1815 with every iteration, or just on the last character? Otherwise, can we
add
> an early return of true here if nPreCount == 0, and then let the compiler
prove
> that the loop must be executed?

Sure, I will have another CL for this.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_jpx_opj.cpp:32: return static_cast<OPJ_SIZE_T>(-1);
On 2016/03/16 18:34:58, Tom Sepez wrote:
> On 2016/03/16 13:23:31, dsinclair wrote:
> > Can you add a TODO, this seems broken. OPJ_SIZE is a typedef for size_t.
> size_t
> > is a unsigned data type. We probably shouldn't be using -1 as a sentinal
value
> > in that case.
> 
> Probably ok along the lines of std::string::npos's usage of the same
technique.

Ack. I digged deeper and found that this is required/used by libopenjpeg code.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Context.cpp:205: if (m_PauseStep <= 1) {
On 2016/03/16 18:34:58, Tom Sepez wrote:
> nit: pity m_PauseStep isn't an enum with a switch here so the compiler could
> prove all cases were covered.

Won't help, a smarter compiler is needed

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Context.cpp:686: uint8_t cSBHUFFFS = 0;
On 2016/03/16 18:34:59, Tom Sepez wrote:
> yup.  Good catch.

Actually these would be assigned at line 698-693. Initialized anyway :p

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_Image.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Image.cpp:668: int32_t xs0 = 0, ys0 = 0, xs1 = 0, ys1 =
0;
On 2016/03/16 18:34:59, Tom Sepez wrote:
> I didn't see how these could be used uninitialized.  Also, can we move these
> down to right before they are used? eg. declare ys0 at 683?
> 
> These might clean up if we combined declaration and initialization using the ?
> operator.

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Image.cpp:678: if (x < -1048576 || x > 1048576 || y <
-1048576 || y > 1048576) {
On 2016/03/16 18:34:59, Tom Sepez wrote:
> nit: they probably mean >= 1048576, since x wont fit in 20 bits if it is
> 1048576.

Ack. I am not certain about the code here, will defer behavior changes from this
CL.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Image.cpp:824: int32_t middleDwords = 0;
On 2016/03/16 18:34:59, Tom Sepez wrote:
> I don't think this needs to persist between blocks, since we initialize it
each
> time.  Maybe use N decls of this at line 828, 920, etc.

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_TrdProc.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_TrdProc.cpp:26: int32_t nVal = 0, nBits = 0;
On 2016/03/16 18:34:59, Tom Sepez wrote:
> These are probably OK since the compiler can't prove that a value will be set
> when passing these by &STRIPT to decodeAsValue().  
> 
> This would benefit from declaring these right before they get clobbered by the
> decode calls.

Very good idea. :)

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_map...
File core/fxcrt/fx_basic_maps.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_map...
core/fxcrt/fx_basic_maps.cpp:42: if ((pAssocRet = m_pHashTable[nBucket]) !=
nullptr)
On 2016/03/16 18:34:59, Tom Sepez wrote:
> what's the warning here?

assignment in condition expression

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_map...
core/fxcrt/fx_basic_maps.cpp:48: if ((pAssocNext = pAssocRet->pNext) == NULL) {
On 2016/03/16 18:34:59, Tom Sepez wrote:
> nit: move assignment to line 47/

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_map...
core/fxcrt/fx_basic_maps.cpp:50: nBucket < m_nHashTableSize; nBucket++) {
On 2016/03/16 18:34:59, Tom Sepez wrote:
> I think if you add && !pAssocNext to the loop condition, you can lose the if
at
> line 48 and write line 51 - 53 as just
>    pAssocNext = m_pHashTable[nBucket];

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_wst...
File core/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_wst...
core/fxcrt/fx_basic_wstring.cpp:644: {
On 2016/03/16 18:34:59, Tom Sepez wrote:
> nit: brace needed here?

Done.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcrt/fx_basic_wst...
core/fxcrt/fx_basic_wstring.cpp:647: lpszStart < lpszEnd) {
On 2016/03/16 18:34:59, Tom Sepez wrote:
> This should be fine.  Not sure we want to write it this way to make the
compiler
> happy.

For pdfium code, this warning is not that useful. But for some other cases,
people sometimes accidentally type "=" instead of "==". We have just a few (3 or
4?) cases like this to clean up. So I am fine following chromium code on this.

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_co...
File core/fxge/dib/fx_dib_composite.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_co...
core/fxge/dib/fx_dib_composite.cpp:212: switch (blend_mode) {
On 2016/03/16 18:34:59, Tom Sepez wrote:
> nit: again, if this were an enum, the compiler might be able to prove that we
> cover all cases.

Ditto

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
File core/fxge/dib/fx_dib_main.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
core/fxge/dib/fx_dib_main.cpp:1239: ASSERT(!IsAlphaMask());
On 2016/03/16 18:34:59, Tom Sepez wrote:
> We can lose the assert here since we handle it at 1240

assert is for debugging purpose? leave it here for now.

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/dib/fx_dib_ma...
core/fxge/dib/fx_dib_main.cpp:1245: int bc = 0, bm = 0, by = 0, bk = 0;
On 2016/03/16 18:34:59, Tom Sepez wrote:
> Stunning lack of duplication between the isCmykImage and !isCmykImage paths. 
> I'd suggest splitting this into two helper functions.

Acknowledged.

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

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/ge/fx_ge_font...
core/fxge/ge/fx_ge_font.cpp:525: return static_cast<FX_DWORD>(-1);
On 2016/03/16 18:34:59, Tom Sepez wrote:
> On 2016/03/16 17:51:31, Wei Li wrote:
> > On 2016/03/16 13:23:32, dsinclair wrote:
> > > Ditto: FX_DWORD is a uint64
> > 
> > Done.
> Nah, s'awwright.

Now I checked more carefully. Looks like -1 used as a sentinel for charcode as
well.

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/win32/fx_win3...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/win32/fx_win3...
core/fxge/win32/fx_win32_gdipext.cpp:1004: switch (pGraphState->m_LineJoin) {
On 2016/03/16 18:34:59, Tom Sepez wrote:
> again, using an enum might prove coverage.

Start to feel too many false alarms? :)

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fpdfppo.cpp
File fpdfsdk/fpdfppo.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fpdfppo.cpp#new...
fpdfsdk/fpdfppo.cpp:88: FX_DWORD Kidsobjnum =
pDestPDFDoc->AddIndirectObject(pNewKids);
On 2016/03/16 18:35:00, Tom Sepez wrote:
> no kidding. !!! :)

You bet :)

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fpdfview.cpp
File fpdfsdk/fpdfview.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fpdfview.cpp#ne...
fpdfsdk/fpdfview.cpp:286: switch (err) {
On 2016/03/16 18:35:00, Tom Sepez wrote:
> Strage, we're not missing coverage for one of these.  And it still complains?

Acknowledged.

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fxedit/fxet_ap.cpp
File fpdfsdk/fxedit/fxet_ap.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fxedit/fxet_ap....
fpdfsdk/fxedit/fxet_ap.cpp:21: FX_DWORD dwCharCode = 0;
On 2016/03/16 18:35:00, Tom Sepez wrote:
> Init shouldn't be required given lines 23-26

Acknowledged.

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/fxedit/fxet_ap....
fpdfsdk/fxedit/fxet_ap.cpp:22: 
On 2016/03/16 18:35:00, Tom Sepez wrote:
> nit: no blank line here.

Acknowledged.

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

https://codereview.chromium.org/1801383002/diff/20001/fpdfsdk/javascript/Docu...
fpdfsdk/javascript/Document.cpp:1462: FX_DWORD charcode =
static_cast<FX_DWORD>(-1);
On 2016/03/16 18:35:00, Tom Sepez wrote:
> On 2016/03/16 17:51:32, Wei Li wrote:
> > On 2016/03/16 13:23:32, dsinclair wrote:
> > > ditto
> > 
> > Done.
> 
> meh.

Ditto

[Tom Sepez, 2016-03-17 16:40:06]

Getting much better.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
File core/fpdfapi/fpdf_render/fpdf_render_text.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfapi/fpdf_rende...
core/fpdfapi/fpdf_render/fpdf_render_text.cpp:140: if
(FXSYS_fabs(image_matrix.b) < FXSYS_fabs(image_matrix.a) / 100 &&
On 2016/03/17 02:24:05, Wei Li wrote:
> On 2016/03/16 18:34:58, Tom Sepez wrote:
> > Do we know if this is ever false? If we could somehow remove it, the
> > initializations wouldn't be required.  I can't see how it works otherwise.
> 
> Yes, it could be true or false. Tricky thing here is that |left| is passed by
> reference as an output argument in line 170. 

Acknowledged.

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp
File core/fpdfdoc/doc_ap.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fpdfdoc/doc_ap.cpp...
core/fpdfdoc/doc_ap.cpp:307: if (pDRDict && (pDRFontDict =
pDRDict->GetDictBy("Font")) != nullptr) {
On 2016/03/16 17:51:31, Wei Li wrote:
> On 2016/03/16 13:23:31, dsinclair wrote:
> > What was the issue with this one?
> 
> 4706, assignment within conditional expression.

Acknowledged.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
File core/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/codec/fx_c...
core/fxcodec/codec/fx_codec_jpx_opj.cpp:32: return static_cast<OPJ_SIZE_T>(-1);
On 2016/03/17 02:24:05, Wei Li wrote:
> On 2016/03/16 18:34:58, Tom Sepez wrote:
> > On 2016/03/16 13:23:31, dsinclair wrote:
> > > Can you add a TODO, this seems broken. OPJ_SIZE is a typedef for size_t.
> > size_t
> > > is a unsigned data type. We probably shouldn't be using -1 as a sentinal
> value
> > > in that case.
> > 
> > Probably ok along the lines of std::string::npos's usage of the same
> technique.
> 
> Ack. I digged deeper and found that this is required/used by libopenjpeg code.

Acknowledged.

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
File core/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxcodec/jbig2/JBig...
core/fxcodec/jbig2/JBig2_Context.cpp:205: if (m_PauseStep <= 1) {
On 2016/03/17 02:24:05, Wei Li wrote:
> On 2016/03/16 18:34:58, Tom Sepez wrote:
> > nit: pity m_PauseStep isn't an enum with a switch here so the compiler could
> > prove all cases were covered.
> 
> Won't help, a smarter compiler is needed

Acknowledged.

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/win32/fx_win3...
File core/fxge/win32/fx_win32_gdipext.cpp (right):

https://codereview.chromium.org/1801383002/diff/20001/core/fxge/win32/fx_win3...
core/fxge/win32/fx_win32_gdipext.cpp:1004: switch (pGraphState->m_LineJoin) {
On 2016/03/17 02:24:06, Wei Li wrote:
> On 2016/03/16 18:34:59, Tom Sepez wrote:
> > again, using an enum might prove coverage.
> 
> Start to feel too many false alarms? :)

Acknowledged.

https://codereview.chromium.org/1801383002/diff/120001/core/fxcodec/jbig2/JBi...
File core/fxcodec/jbig2/JBig2_Image.cpp (right):

https://codereview.chromium.org/1801383002/diff/120001/core/fxcodec/jbig2/JBi...
core/fxcodec/jbig2/JBig2_Image.cpp:671: if (x < -1048576 || x > 1048576 || y <
-1048576 || y > 1048576) {
Ok, let's add a TODO here to check on 1048576 vs 1048575.

https://codereview.chromium.org/1801383002/diff/120001/core/fxcodec/jbig2/JBi...
File core/fxcodec/jbig2/JBig2_TrdProc.cpp (right):

https://codereview.chromium.org/1801383002/diff/120001/core/fxcodec/jbig2/JBi...
core/fxcodec/jbig2/JBig2_TrdProc.cpp:100: int32_t RDWI;
Do these require initializers per the discussion above?

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ma...
File core/fxcrt/fx_basic_maps.cpp (right):

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ma...
core/fxcrt/fx_basic_maps.cpp:49: if (!pAssocNext) {
Do we still need this if given the check at 51?

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ma...
core/fxcrt/fx_basic_maps.cpp:50: for (FX_DWORD nBucket =
(HashKey(pAssocRet->key) % m_nHashTableSize) + 1;
nit: [super nit] Maybe this reads cleaner as 
CAssoc* pAssocNext = pAssocRet->pNext;
FX_DWORD nBucket = (HashKey(pAssocRet->key) % m_nHashTableSize) + 1;
while (!pAssocNext && nBucket < m_nHashTableSize)
  pAssocNext = m_pHashTable[nBucket++];

but double-check my logic.

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ws...
File core/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ws...
core/fxcrt/fx_basic_wstring.cpp:644: while ((lpszTarget =
FXSYS_wcsstr(lpszStart, lpszOld)) != nullptr &&
nit: again, I think this reads cleaner as a while loop that only checks the
(lpszStart < lpszEnd) at top. Then it looks like lsszTarget can become local to
the loop body.

while (lpszStart < lpszEnd) {
  FX_WCHAR* lpszTarget = FXSYS_wcsstr(lpszStart, lpszOld);
  if (!lpszTarget)
    break;
  ...

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ws...
core/fxcrt/fx_basic_wstring.cpp:667: {
nit: braces?

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ws...
core/fxcrt/fx_basic_wstring.cpp:668: while ((lpszTarget =
FXSYS_wcsstr(lpszStart, lpszOld)) != nullptr &&
nit: re-write as discussed above.

[Wei Li, 2016-03-18 17:42:39]

Thanks for the review.

https://codereview.chromium.org/1801383002/diff/120001/core/fxcodec/jbig2/JBi...
File core/fxcodec/jbig2/JBig2_Image.cpp (right):

https://codereview.chromium.org/1801383002/diff/120001/core/fxcodec/jbig2/JBi...
core/fxcodec/jbig2/JBig2_Image.cpp:671: if (x < -1048576 || x > 1048576 || y <
-1048576 || y > 1048576) {
On 2016/03/17 16:40:05, Tom Sepez wrote:
> Ok, let's add a TODO here to check on 1048576 vs 1048575.

Done.

https://codereview.chromium.org/1801383002/diff/120001/core/fxcodec/jbig2/JBi...
File core/fxcodec/jbig2/JBig2_TrdProc.cpp (right):

https://codereview.chromium.org/1801383002/diff/120001/core/fxcodec/jbig2/JBi...
core/fxcodec/jbig2/JBig2_TrdProc.cpp:100: int32_t RDWI;
On 2016/03/17 16:40:05, Tom Sepez wrote:
> Do these require initializers per the discussion above?

No need, looks like when they are declared immediately before they used as
output arguments, they are fine.

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ma...
File core/fxcrt/fx_basic_maps.cpp (right):

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ma...
core/fxcrt/fx_basic_maps.cpp:49: if (!pAssocNext) {
On 2016/03/17 16:40:05, Tom Sepez wrote:
> Do we still need this if given the check at 51?

Done.

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ma...
core/fxcrt/fx_basic_maps.cpp:50: for (FX_DWORD nBucket =
(HashKey(pAssocRet->key) % m_nHashTableSize) + 1;
On 2016/03/17 16:40:05, Tom Sepez wrote:
> nit: [super nit] Maybe this reads cleaner as 
> CAssoc* pAssocNext = pAssocRet->pNext;
> FX_DWORD nBucket = (HashKey(pAssocRet->key) % m_nHashTableSize) + 1;
> while (!pAssocNext && nBucket < m_nHashTableSize)
>   pAssocNext = m_pHashTable[nBucket++];
> 
> but double-check my logic.  

I agree with you on the readability for the loop. On the other hand, the nBucket
can be confined within the loop. So how about leave it for now?

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ws...
File core/fxcrt/fx_basic_wstring.cpp (right):

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ws...
core/fxcrt/fx_basic_wstring.cpp:644: while ((lpszTarget =
FXSYS_wcsstr(lpszStart, lpszOld)) != nullptr &&
On 2016/03/17 16:40:05, Tom Sepez wrote:
> nit: again, I think this reads cleaner as a while loop that only checks the
> (lpszStart < lpszEnd) at top. Then it looks like lsszTarget can become local
to
> the loop body.
> 
> while (lpszStart < lpszEnd) {
>   FX_WCHAR* lpszTarget = FXSYS_wcsstr(lpszStart, lpszOld);
>   if (!lpszTarget)
>     break;
>   ...

Absolutely agree with you. I originally planned to do this, but thought some
people might like less lines. :)

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ws...
core/fxcrt/fx_basic_wstring.cpp:667: {
On 2016/03/17 16:40:05, Tom Sepez wrote:
> nit: braces?

Done.

https://codereview.chromium.org/1801383002/diff/120001/core/fxcrt/fx_basic_ws...
core/fxcrt/fx_basic_wstring.cpp:668: while ((lpszTarget =
FXSYS_wcsstr(lpszStart, lpszOld)) != nullptr &&
On 2016/03/17 16:40:05, Tom Sepez wrote:
> nit: re-write as discussed above.

Done.

[Tom Sepez, 2016-03-18 20:36:34]

lgtm

[Wei Li, 2016-03-21 18:20:52]

Description was changed from

==========
Re-enable several MSVC warnings

Re-enable the following warnings:
 4245: signed/unsigned conversion mismatch;
 4310: cast may truncate data;
 4389: operator on signed/unsigned mismatch;
 4701: use potentially uninitialized local variable;
 4706: assignment within conditional expression

Clean up the code to avoid those warnings.

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

to

==========
Re-enable several MSVC warnings

Re-enable the following warnings:
 4245: signed/unsigned conversion mismatch;
 4310: cast may truncate data;
 4389: operator on signed/unsigned mismatch;
 4701: use potentially uninitialized local variable;
 4706: assignment within conditional expression

Clean up the code to avoid those warnings.

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

Committed:
https://pdfium.googlesource.com/pdfium/+/d4e8f1222ca17b57ac74019b2fc3706e1192...
==========

[Wei Li, 2016-03-21 18:20:53]

Committed patchset #7 (id:180001) manually as
d4e8f1222ca17b57ac74019b2fc3706e1192645c (presubmit successful).