Cleanup JBig2_GeneralDecoder.cpp.

Cleanup JBig2_GeneralDecoder.cpp.

- FX_Alloc() can't fail and return.
- Use unique_ptr / remove gotos.
- NULL -> nullptr.
- Combine common code.

R=tsepez@chromium.org

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

[Lei Zhang, 2015-09-11 23:16:13]

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

[Lei Zhang, 2015-09-11 23:16:14]

Each patch set is (mostly) one mechanical step. So it's more obvious how we
arrived at patch set 5.

[Tom Sepez, 2015-09-11 23:36:21]

This is for PS#1

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

https://codereview.chromium.org/1337823003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:166:
nonstd::unique_ptr<CJBig2_Image> GBREG(new CJBig2_Image(GBW, GBH));
Didn't see any early returns here?  => raw pointer OK.
Alternatively, do we want the same NULL check here as above?

https://codereview.chromium.org/1337823003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:303:
nonstd::unique_ptr<CJBig2_Image> GBREG(new CJBig2_Image(GBW, GBH));
ditto, and in several other places in this file.

https://codereview.chromium.org/1337823003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1220:
nonstd::unique_ptr<CJBig2_ArithDecoder> pArithDecoder(
want to add a {} block here to force pArithDecoder to go out of scope early like
in line 1221(OLD).

https://codereview.chromium.org/1337823003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1562:
nonstd::unique_ptr<CJBig2_SymbolDict> pDict(new CJBig2_SymbolDict());
why this move up from old line 1800?  My old eyes didn't see an earlier
reference?

[Tom Sepez, 2015-09-11 23:38:38]

PS#2 diffs OK.

[Tom Sepez, 2015-09-11 23:41:40]

This is PS#3

https://codereview.chromium.org/1337823003/diff/40001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp (right):

https://codereview.chromium.org/1337823003/diff/40001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1540:
nonstd::unique_ptr<CJBig2_ArithIntDecoder> IADH(new CJBig2_ArithIntDecoder());
nit: if you lost the () in the new expression, I bet these all fit on one line
which makes my eyes much happier.

[Tom Sepez, 2015-09-11 23:46:03]

PS#4

https://codereview.chromium.org/1337823003/diff/60001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp (right):

https://codereview.chromium.org/1337823003/diff/60001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:37: return (GBAT[0] == 3) &&
(GBAT[1] == -1) && (GBAT[2] == -3) &&
nit: unnecessary () here and below.

https://codereview.chromium.org/1337823003/diff/60001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:56: if (UseTemplate0Opt3())
might go with ? operator here, which leads to single return, which makes the
"else after return" here more obvious.

https://codereview.chromium.org/1337823003/diff/60001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:600: (GRAT[3] == -1) &&
(GRREFERENCEDX == 0) &&
migt be tempted to put this logic into its own private method, as above, even if
its only used one, to make the code cleaner.

[Tom Sepez, 2015-09-11 23:46:59]

PS#5 OK

https://codereview.chromium.org/1337823003/diff/80001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp (right):

https://codereview.chromium.org/1337823003/diff/80001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:791: if (line3_r_ok) {
yeah, much better!

[Lei Zhang, 2015-09-12 01:19:28]

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

https://codereview.chromium.org/1337823003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:166:
nonstd::unique_ptr<CJBig2_Image> GBREG(new CJBig2_Image(GBW, GBH));
On 2015/09/11 23:36:21, Tom Sepez wrote:
> Didn't see any early returns here?  => raw pointer OK.
> Alternatively, do we want the same NULL check here as above?

We probably should standardize on that NULL check. Maybe in another CL if you
don't mind?

In any case, I'd like to use a unique_ptr because:
a) We might add early returns
b) Someday when we have a nicer unique_ptr, we can return
nonstd::unique_ptr<CJBig2_Image> instead of a raw pointer to indicate transfer
of ownership.

https://codereview.chromium.org/1337823003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1220:
nonstd::unique_ptr<CJBig2_ArithDecoder> pArithDecoder(
On 2015/09/11 23:36:21, Tom Sepez wrote:
> want to add a {} block here to force pArithDecoder to go out of scope early
like
> in line 1221(OLD).

Done.

https://codereview.chromium.org/1337823003/diff/1/core/src/fxcodec/jbig2/JBig...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1562:
nonstd::unique_ptr<CJBig2_SymbolDict> pDict(new CJBig2_SymbolDict());
On 2015/09/11 23:36:21, Tom Sepez wrote:
> why this move up from old line 1800?  My old eyes didn't see an earlier
> reference?

Can't declare a variable down there because of gotos in between. I'll declare it
here and use it below. The gotos will eventually go away, and then we can move a
bunch of declaration down to where they are first used.

https://codereview.chromium.org/1337823003/diff/40001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp (right):

https://codereview.chromium.org/1337823003/diff/40001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_GeneralDecoder.cpp:1540:
nonstd::unique_ptr<CJBig2_ArithIntDecoder> IADH(new CJBig2_ArithIntDecoder());
On 2015/09/11 23:41:40, Tom Sepez wrote:
> nit: if you lost the () in the new expression, I bet these all fit on one line
> which makes my eyes much happier.

Done.

[Tom Sepez, 2015-09-14 16:58:43]

LGTM.

> b) Someday when we have a nicer unique_ptr, we can return
> nonstd::unique_ptr<CJBig2_Image> instead of a raw pointer to indicate transfer
> of ownership.
> 

Given the continuing lag on c++11 stdlib, I'm thinking its time I put a move
constructor into our placeholder, esp. since I like the pattern you describe
a lot.

[Lei Zhang, 2015-09-15 08:12:58]

Committed patchset #7 (id:110001) manually as
d15cda90f598ce52fa601552577ef7fd8a89fce3 (presubmit successful).