Remove gotos in JBig2 code.

Remove gotos in JBig2 code.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/8a9ce5778e0789beb7ba15a62c592a353a51bcdd

[Lei Zhang, 2015-10-05 01:40:56]

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

[Lei Zhang, 2015-10-05 01:40:56]

Many of the FX_Alloc()s can probably be replaced with std::vector, but that's
for another pass.

[Tom Sepez, 2015-10-05 16:34:32]

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:441: FX_DWORD dwTemp;
Can we scope this to the next block?  That way, it is obvious that it can't be
used uninitialized later.  There are going to be a few other dwTemp declarations
required in other blocks as a result.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:444: dwTemp = 8;
nit: this becomes a nice ? expr when combined with the decl.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:448: for (int32_t i = 0; i <
(int32_t)dwTemp; i++) {
nit: why not FX_DWORD i and lose the cast? also ++i.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:472: CJBig2_Segment* pSeg = nullptr;
nit: pSeg maybe scopes tighter?  I think it's always re-assigned when used
later, but doublecheck.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:580: dwTemp =
pSymbolDictDecoder->SDRTEMPLATE ? 1 << 10 : 1 << 13;
nit: 8192 and 1024 as in line 574, but let's write it different and in a
different order without an  == 1.  Is this a bool? Are the tests for == 1 above
wrong?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:602: for
(std::list<CJBig2_CachePair>::iterator it = m_pSymbolDictCache->begin();
nit: not your fault, but auto now that it's allowed.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:614: CJBig2_ArithDecoder* pArithDecoder
=
nit: this could be unique_ptr and lose the delete at 618?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:655: if ((parseRegionInfo(&ri) !=
JBIG2_SUCCESS) ||
nit: overparenthesized

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:677: uint8_t cSBHUFFFS;
nit: Yeah, these are used later only when pTRD->SBHUFF, but they sure like
uninitialized.  Hmmm.  Comment?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:712: CJBig2_Segment* pSeg;
does this scope tighter even if it costs a couple of parallel decls?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:980: CJBig2_ArithDecoder* pArithDecoder
=
nit: unique_ptr here, too?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1008:
(m_pStream->readInteger((FX_DWORD*)&pHRD->HGX) != 0) ||
nit: theoretically illegal casts here.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1031: return JBIG2_ERROR_FATAL;
Does this leak for the non-16 case?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1035: return JBIG2_ERROR_FATAL;
ditto

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1048: CJBig2_ArithDecoder*
pArithDecoder =
nonstd unique ptr?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1083: m_pGRD.reset(new CJBig2_GRDProc);
maybe a local unique_ptr and nonstd::move() at the end of the block avoids a lot
of these resets to null.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1150: delete m_pArithDecoder;
should m_pArithDecoder be unique_ptr?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1234: FX_DWORD dwTemp =
pGRRD->GRTEMPLATE ? 1 << 10 : 1 << 13;
nit: our friends 1k and 8k again.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1241: delete pArithDecoder;
same pattern here as above

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1314: if ((nBits == runcodes_len[j]) &&
(nVal == runcodes[j])) {
nit: overparenthesized.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_GsidProc.cpp (right):

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GsidProc.cpp:125: for (FX_DWORD y = 0; y < GSH;
y++) {
nit: ++x, and ++y below.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp (right):

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:152: int* LENCOUNT = FX_Alloc(int,
(LENMAX + 1));
maybe nonstd::unique_ptr here and at 154 to lose the FX_FREES below.

[Lei Zhang, 2015-10-07 07:51:12]

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:441: FX_DWORD dwTemp;
On 2015/10/05 16:34:31, Tom Sepez wrote:
> Can we scope this to the next block?  That way, it is obvious that it can't be
> used uninitialized later.  There are going to be a few other dwTemp
declarations
> required in other blocks as a result.

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:444: dwTemp = 8;
On 2015/10/05 16:34:32, Tom Sepez wrote:
> nit: this becomes a nice ? expr when combined with the decl.

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:448: for (int32_t i = 0; i <
(int32_t)dwTemp; i++) {
On 2015/10/05 16:34:32, Tom Sepez wrote:
> nit: why not FX_DWORD i and lose the cast? also ++i.

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:472: CJBig2_Segment* pSeg = nullptr;
On 2015/10/05 16:34:32, Tom Sepez wrote:
> nit: pSeg maybe scopes tighter?  I think it's always re-assigned when used
> later, but doublecheck.

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:580: dwTemp =
pSymbolDictDecoder->SDRTEMPLATE ? 1 << 10 : 1 << 13;
On 2015/10/05 16:34:31, Tom Sepez wrote:
> nit: 8192 and 1024 as in line 574, but let's write it different and in a
> different order without an  == 1.  Is this a bool? Are the tests for == 1
above
> wrong?

I refactored some of this code, but I'll fix the bools in another pass.
Otherwise this CL may never end.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:602: for
(std::list<CJBig2_CachePair>::iterator it = m_pSymbolDictCache->begin();
On 2015/10/05 16:34:32, Tom Sepez wrote:
> nit: not your fault, but auto now that it's allowed.

another day... maybe dml@ will get to this.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:614: CJBig2_ArithDecoder* pArithDecoder
=
On 2015/10/05 16:34:31, Tom Sepez wrote:
> nit: this could be unique_ptr and lose the delete at 618?

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:655: if ((parseRegionInfo(&ri) !=
JBIG2_SUCCESS) ||
On 2015/10/05 16:34:31, Tom Sepez wrote:
> nit: overparenthesized

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:677: uint8_t cSBHUFFFS;
On 2015/10/05 16:34:32, Tom Sepez wrote:
> nit: Yeah, these are used later only when pTRD->SBHUFF, but they sure like
> uninitialized.  Hmmm.  Comment?

they are only used when pTRD->SBHUFF == 1.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:712: CJBig2_Segment* pSeg;
On 2015/10/05 16:34:32, Tom Sepez wrote:
> does this scope tighter even if it costs a couple of parallel decls?

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:980: CJBig2_ArithDecoder* pArithDecoder
=
On 2015/10/05 16:34:32, Tom Sepez wrote:
> nit: unique_ptr here, too?

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1008:
(m_pStream->readInteger((FX_DWORD*)&pHRD->HGX) != 0) ||
On 2015/10/05 16:34:31, Tom Sepez wrote:
> nit: theoretically illegal casts here. 

file a bug so I don't forget these?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1031: return JBIG2_ERROR_FATAL;
On 2015/10/05 16:34:32, Tom Sepez wrote:
> Does this leak for the non-16 case?

What are we leaking? |pSeg|? I don't think we have ownership here.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1035: return JBIG2_ERROR_FATAL;
On 2015/10/05 16:34:32, Tom Sepez wrote:
> ditto

Ditto, no leak?

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1048: CJBig2_ArithDecoder*
pArithDecoder =
On 2015/10/05 16:34:31, Tom Sepez wrote:
> nonstd unique ptr?

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1083: m_pGRD.reset(new CJBig2_GRDProc);
On 2015/10/05 16:34:31, Tom Sepez wrote:
> maybe a local unique_ptr and nonstd::move() at the end of the block avoids a
lot
> of these resets to null.

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1150: delete m_pArithDecoder;
On 2015/10/05 16:34:31, Tom Sepez wrote:
> should m_pArithDecoder be unique_ptr?

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1234: FX_DWORD dwTemp =
pGRRD->GRTEMPLATE ? 1 << 10 : 1 << 13;
On 2015/10/05 16:34:32, Tom Sepez wrote:
> nit: our friends 1k and 8k again.

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1241: delete pArithDecoder;
On 2015/10/05 16:34:32, Tom Sepez wrote:
> same pattern here as above 

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1314: if ((nBits == runcodes_len[j]) &&
(nVal == runcodes[j])) {
On 2015/10/05 16:34:31, Tom Sepez wrote:
> nit: overparenthesized.

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_GsidProc.cpp (right):

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GsidProc.cpp:125: for (FX_DWORD y = 0; y < GSH;
y++) {
On 2015/10/05 16:34:32, Tom Sepez wrote:
> nit: ++x, and ++y below.

Done.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp (right):

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_HuffmanTable.cpp:152: int* LENCOUNT = FX_Alloc(int,
(LENMAX + 1));
On 2015/10/05 16:34:32, Tom Sepez wrote:
> maybe nonstd::unique_ptr here and at 154 to lose the FX_FREES below.

Made them std::vectors.

[Tom Sepez, 2015-10-07 17:13:12]

lgtm

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (right):

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1031: return JBIG2_ERROR_FATAL;
On 2015/10/07 07:51:11, Lei Zhang wrote:
> On 2015/10/05 16:34:32, Tom Sepez wrote:
> > Does this leak for the non-16 case?
> 
> What are we leaking? |pSeg|? I don't think we have ownership here.

Acknowledged.

https://codereview.chromium.org/1382613003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_Context.cpp:1035: return JBIG2_ERROR_FATAL;
On 2015/10/07 07:51:12, Lei Zhang wrote:
> On 2015/10/05 16:34:32, Tom Sepez wrote:
> > ditto
> 
> Ditto, no leak?

Acknowledged.

[Lei Zhang, 2015-10-07 17:37:11]

Committed patchset #2 (id:20001) manually as
8a9ce5778e0789beb7ba15a62c592a353a51bcdd (presubmit successful).