Get rid of gotos in CPDF_SyntaxParser and FlateUncompress().

Get rid of gotos in CPDF_SyntaxParser and FlateUncompress().

R=tsepez@chromium.org

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

[Lei Zhang, 2015-09-23 19:22:28]

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

[Lei Zhang, 2015-09-23 19:22:28]

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec_flate.cpp (right):

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:617: void FlateUncompress(const
uint8_t* src_buf,
A bit harder to see what I changed since I moved this function.

- With the gotos gone, I can declare variables closer to where they are used.
- FX_Realloc() can't fail and return NULL, so those checks are gone.
- Call FPDFAPI_FlateEnd(context) on failure.
- Use a unique_ptr for |guess_buf| so there's slightly better tracking of
ownership.

[Tom Sepez, 2015-09-23 21:48:41]

I think the logic is OK

https://codereview.chromium.org/1360103002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1360103002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:597: if (streampos &&
!LoadCrossRefV5(streampos, streampos, FALSE))
nit: maybe return !streampos || LoadCrossRefV5(..);
or not if you think its confusing.

https://codereview.chromium.org/1360103002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2765: if ((limit && pos >=
m_Pos + limit) || !GetCharAt(pos, byte)) {
I would like to have the first half of this || used in some form as the
condition in the for loop at 2739. Also, the getcharat can then be first in the
loop rather than last (unify with line 2738, and note that line's return value
isn't checked) so that we work on a new byte each time rather than the residue
of the last iteration. Let me know if there's a gotcha that prevents such a
form.

https://codereview.chromium.org/1360103002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2769: return -1;
notreached? Only way out of your loop by return? want to break instead at 2766?

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec_flate.cpp (right):

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:625: orig_size ? 10240 : (src_size <
10240 ? 10240 : src_size);
nit: maybe std::min(src_size, 10240)
maybe a const FX_DWORD kMinStepSize = 10240;

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:636: dest_buf = nullptr;
how about we set these to null/0 initially, and then only set them to legit
values right before a successful return?  Then this can be if (!context) return;

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:643: guess_buf.get()[guess_size] =
'\0';
The real std::unique_ptr has operator[]. do we want to do the same before
landing this?  Do we want to new[] rather than mallocating?

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:658: // |avail_buf_size| == 0 case.
nit: not sure this comment adds value.

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:671: guess_buf.reset(new_buf);
nit: how about guess_buf.reset(FX_Realloc(...)); and lose the { }

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:681: uint8_t* new_buf =
ditto

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:706: // |avail_buf_size| == 0 case.
ditto

[Lei Zhang, 2015-09-25 09:33:49]

https://codereview.chromium.org/1360103002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1360103002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:597: if (streampos &&
!LoadCrossRefV5(streampos, streampos, FALSE))
On 2015/09/23 21:48:41, Tom Sepez wrote:
> nit: maybe return !streampos || LoadCrossRefV5(..);
> or not if you think its confusing.

Done.

https://codereview.chromium.org/1360103002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2765: if ((limit && pos >=
m_Pos + limit) || !GetCharAt(pos, byte)) {
On 2015/09/23 21:48:41, Tom Sepez wrote:
> I would like to have the first half of this || used in some form as the
> condition in the for loop at 2739. Also, the getcharat can then be first in
the
> loop rather than last (unify with line 2738, and note that line's return value
> isn't checked) so that we work on a new byte each time rather than the residue
> of the last iteration. Let me know if there's a gotcha that prevents such a
> form.

Done.

https://codereview.chromium.org/1360103002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2769: return -1;
On 2015/09/23 21:48:41, Tom Sepez wrote:
> notreached? Only way out of your loop by return? want to break instead at
2766?

Acknowledged.

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec_flate.cpp (right):

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:625: orig_size ? 10240 : (src_size <
10240 ? 10240 : src_size);
On 2015/09/23 21:48:41, Tom Sepez wrote:
> nit: maybe std::min(src_size, 10240)
> maybe a const FX_DWORD kMinStepSize = 10240;

Done.

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:636: dest_buf = nullptr;
On 2015/09/23 21:48:41, Tom Sepez wrote:
> how about we set these to null/0 initially, and then only set them to legit
> values right before a successful return?  Then this can be if (!context)
return;

Done.

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:643: guess_buf.get()[guess_size] =
'\0';
On 2015/09/23 21:48:41, Tom Sepez wrote:
> The real std::unique_ptr has operator[]. do we want to do the same before
> landing this?  Do we want to new[] rather than mallocating?

There's no new equivalent to FX_Realloc(), right? I would use a std::vector if
we didn't have to send the |guess_buf| owned pointer out via |dest_buf|.

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:658: // |avail_buf_size| == 0 case.
On 2015/09/23 21:48:41, Tom Sepez wrote:
> nit: not sure this comment adds value.

Done.

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:671: guess_buf.reset(new_buf);
On 2015/09/23 21:48:41, Tom Sepez wrote:
> nit: how about guess_buf.reset(FX_Realloc(...)); and lose the { }

No, we get in trouble doing guess_buf.reset() and guess_buf.release() at the
same time.

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:681: uint8_t* new_buf =
On 2015/09/23 21:48:41, Tom Sepez wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:706: // |avail_buf_size| == 0 case.
On 2015/09/23 21:48:41, Tom Sepez wrote:
> ditto

Done.

[Tom Sepez, 2015-09-25 16:25:35]

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
File core/src/fxcodec/codec/fx_codec_flate.cpp (right):

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:643: guess_buf.get()[guess_size] =
'\0';
On 2015/09/25 09:33:48, Lei Zhang wrote:
> On 2015/09/23 21:48:41, Tom Sepez wrote:
> > The real std::unique_ptr has operator[]. do we want to do the same before
> > landing this?  Do we want to new[] rather than mallocating?
> 
> There's no new equivalent to FX_Realloc(), right? I would use a std::vector if
> we didn't have to send the |guess_buf| owned pointer out via |dest_buf|.

ah.

https://codereview.chromium.org/1360103002/diff/1/core/src/fxcodec/codec/fx_c...
core/src/fxcodec/codec/fx_codec_flate.cpp:671: guess_buf.reset(new_buf);
On 2015/09/25 09:33:48, Lei Zhang wrote:
> On 2015/09/23 21:48:41, Tom Sepez wrote:
> > nit: how about guess_buf.reset(FX_Realloc(...)); and lose the { }
> 
> No, we get in trouble doing guess_buf.reset() and guess_buf.release() at the
> same time.

Acknowledged.

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

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:20: int
_CompareFileSize(const void* p1, const void* p2) {
nit: can we lose the leading _ (it's namespaced now, and underbar CAPITAL is
technically reserved for use by C++ itself).

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:21: FX_FILESIZE ret =
(*(FX_FILESIZE*)p1) - (*(FX_FILESIZE*)p2);
nit: overparenthesized.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:24: return ret > 0 ? 1 : -1;
Can you confirm if we're only passing this to qsort()/bsearch()?  If so, the
typical thing is just to return *p1 - *p2 (casted as needed) and avoid the
branch.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:46: if (!pObj)
nit: how about 

if (pObj && pObj->GetType() == PDFOBJ_NUMBER)
  return ...;

return 0;

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:59: return pObj ?
(pObj->GetType() == iType) : true;
nit: return !pObj || pObj->GetType() == itype;

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:62: FX_DWORD
_GetVarInt(const uint8_t* p, int32_t n) {
nit: leading _

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:503: void* pResult =
nit: Local not needed

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:510: void* pStreamResult =
FXSYS_bsearch(&streampos, m_SortedOffset.GetData(),
nit: Thanks for fixing the shadowing, but local still not needed.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:584: void* pRefResult =
nit: third time.  Maybe a helper method.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1739:
CFX_AutoRestorer<FX_FILESIZE> save_pos(&m_Pos);
nice.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2736: for (FX_FILESIZE pos =
m_Pos; limit && pos >= pos_limit; ++pos) {
I think you need to negate your test.

[Lei Zhang, 2015-09-25 22:04:07]

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

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:20: int
_CompareFileSize(const void* p1, const void* p2) {
On 2015/09/25 16:25:34, Tom Sepez wrote:
> nit: can we lose the leading _ (it's namespaced now, and underbar CAPITAL is
> technically reserved for use by C++ itself).

Done.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:21: FX_FILESIZE ret =
(*(FX_FILESIZE*)p1) - (*(FX_FILESIZE*)p2);
On 2015/09/25 16:25:34, Tom Sepez wrote:
> nit: overparenthesized.

Done.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:24: return ret > 0 ? 1 : -1;
On 2015/09/25 16:25:35, Tom Sepez wrote:
> Can you confirm if we're only passing this to qsort()/bsearch()?  If so, the
> typical thing is just to return *p1 - *p2 (casted as needed) and avoid the
> branch.

Done.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:46: if (!pObj)
On 2015/09/25 16:25:35, Tom Sepez wrote:
> nit: how about 
> 
> if (pObj && pObj->GetType() == PDFOBJ_NUMBER)
>   return ...;
> 
> return 0;

Done.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:59: return pObj ?
(pObj->GetType() == iType) : true;
On 2015/09/25 16:25:35, Tom Sepez wrote:
> nit: return !pObj || pObj->GetType() == itype;

Done.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:62: FX_DWORD
_GetVarInt(const uint8_t* p, int32_t n) {
On 2015/09/25 16:25:35, Tom Sepez wrote:
> nit: leading _

Done.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:503: void* pResult =
On 2015/09/25 16:25:35, Tom Sepez wrote:
> nit: Local not needed

Done.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:510: void* pStreamResult =
FXSYS_bsearch(&streampos, m_SortedOffset.GetData(),
On 2015/09/25 16:25:35, Tom Sepez wrote:
> nit: Thanks for fixing the shadowing, but local still not needed.

Done.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:584: void* pRefResult =
On 2015/09/25 16:25:35, Tom Sepez wrote:
> nit: third time.  Maybe a helper method.

Done.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1739:
CFX_AutoRestorer<FX_FILESIZE> save_pos(&m_Pos);
On 2015/09/25 16:25:34, Tom Sepez wrote:
> nice.

Acknowledged.

https://codereview.chromium.org/1360103002/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2736: for (FX_FILESIZE pos =
m_Pos; limit && pos >= pos_limit; ++pos) {
On 2015/09/25 16:25:35, Tom Sepez wrote:
> I think you need to negate your test.

Done.

[Lei Zhang, 2015-10-02 02:08:03]

ping

[Tom Sepez, 2015-10-02 17:23:46]

lgtm

https://codereview.chromium.org/1360103002/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1360103002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:22: return ret == 0 ? 0 :
ret;
nit: this is just 
 return ret;
which then gets combined with the previous line into
  return *(FX_FILESIZE*)p1 - *(FX_FILESIZE*)p2;

[Lei Zhang, 2015-10-02 18:26:53]

https://codereview.chromium.org/1360103002/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1360103002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:22: return ret == 0 ? 0 :
ret;
On 2015/10/02 17:23:45, Tom Sepez wrote:
> nit: this is just 
>  return ret;
> which then gets combined with the previous line into
>   return *(FX_FILESIZE*)p1 - *(FX_FILESIZE*)p2;

Done.

[Lei Zhang, 2015-10-02 18:27:01]

Committed patchset #6 (id:100001) manually as
b8a074711108baf04017d86207e08c19ff327d8e (presubmit successful).