Cleanup JBig2_ArithIntDecoder.

Cleanup JBig2_ArithIntDecoder.

R=tsepez@chromium.org

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

[Lei Zhang, 2015-09-28 17:33:20]

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

[Lei Zhang, 2015-09-28 17:33:21]

I changed the decode() calls to be less consistent - if decode() can never fail,
just return the decoded value. Otherwise, use an out param and return a bool,
like in patch set 9... where we also find that decode() returns 0 or 1, but the
caller checks for -1. @_@

[Tom Sepez, 2015-09-28 21:38:47]

On 2015/09/28 17:33:21, Lei Zhang wrote:
> I changed the decode() calls to be less consistent - if decode() can never
fail,
> just return the decoded value. Otherwise, use an out param and return a bool,
> like in patch set 9... where we also find that decode() returns 0 or 1, but
the
> caller checks for -1. @_@
Might be better to just make the return a void if it can't fail, and pass by
output.  Then we never mistake one for the other.

The ArithIntDecoder changes are making my head hurt, but I think they are right.

[Tom Sepez, 2015-09-28 21:38:55]

https://codereview.chromium.org/1359013003/diff/180001/core/src/fxcodec/jbig2...
File core/src/fxcodec/jbig2/JBig2_ArithIntDecoder.cpp (right):

https://codereview.chromium.org/1359013003/diff/180001/core/src/fxcodec/jbig2...
core/src/fxcodec/jbig2/JBig2_ArithIntDecoder.cpp:13: void ShiftOr(int* val, int
bitwise_or_val) {
nit: maybe this reads better if it returned the val, rather than using it as an
in-out parameter. Later you write

prev = ShiftOr(prev, D)

rather than

ShiftOr(&prev, D)

[Lei Zhang, 2015-09-29 00:51:48]

Patchset #11 (id:200001) has been deleted

[Lei Zhang, 2015-09-29 00:58:22]

https://codereview.chromium.org/1359013003/diff/180001/core/src/fxcodec/jbig2...
File core/src/fxcodec/jbig2/JBig2_ArithIntDecoder.cpp (right):

https://codereview.chromium.org/1359013003/diff/180001/core/src/fxcodec/jbig2...
core/src/fxcodec/jbig2/JBig2_ArithIntDecoder.cpp:13: void ShiftOr(int* val, int
bitwise_or_val) {
On 2015/09/28 21:38:55, Tom Sepez wrote:
> nit: maybe this reads better if it returned the val, rather than using it as
an
> in-out parameter. Later you write
> 
> prev = ShiftOr(prev, D)
> 
> rather than
> 
> ShiftOr(&prev, D)

Done.

[Tom Sepez, 2015-09-29 17:46:36]

https://codereview.chromium.org/1359013003/diff/220001/core/src/fxcodec/jbig2...
File core/src/fxcodec/jbig2/JBig2_SddProc.cpp (right):

https://codereview.chromium.org/1359013003/diff/220001/core/src/fxcodec/jbig2...
core/src/fxcodec/jbig2/JBig2_SddProc.cpp:212: IAID->decode(pArithDecoder,
reinterpret_cast<int*>(&IDI));
Technically, an illegal cast, since there's no reason to believe an FX_DWORD and
an int are related in any manner.  Can we change the out parameter type to match
and lose the cast?

[Lei Zhang, 2015-09-30 05:12:20]

https://codereview.chromium.org/1359013003/diff/220001/core/src/fxcodec/jbig2...
File core/src/fxcodec/jbig2/JBig2_SddProc.cpp (right):

https://codereview.chromium.org/1359013003/diff/220001/core/src/fxcodec/jbig2...
core/src/fxcodec/jbig2/JBig2_SddProc.cpp:212: IAID->decode(pArithDecoder,
reinterpret_cast<int*>(&IDI));
On 2015/09/29 17:46:36, Tom Sepez wrote:
> Technically, an illegal cast, since there's no reason to believe an FX_DWORD
and
> an int are related in any manner.  Can we change the out parameter type to
match
> and lose the cast?

Done.

[Tom Sepez, 2015-09-30 20:09:51]

https://codereview.chromium.org/1359013003/diff/260001/core/src/fxcodec/jbig2...
File core/src/fxcodec/jbig2/JBig2_TrdProc.cpp (right):

https://codereview.chromium.org/1359013003/diff/260001/core/src/fxcodec/jbig2...
core/src/fxcodec/jbig2/JBig2_TrdProc.cpp:302: if (!IAIT->decode(pArithDecoder,
&nVal)) {
Same thing here.  Can we get rid of the nVal temporary entirely and get
consistent types?

[Lei Zhang, 2015-10-01 05:59:36]

Patchset #14 (id:280001) has been deleted

[Lei Zhang, 2015-10-01 06:01:59]

https://codereview.chromium.org/1359013003/diff/260001/core/src/fxcodec/jbig2...
File core/src/fxcodec/jbig2/JBig2_TrdProc.cpp (right):

https://codereview.chromium.org/1359013003/diff/260001/core/src/fxcodec/jbig2...
core/src/fxcodec/jbig2/JBig2_TrdProc.cpp:302: if (!IAIT->decode(pArithDecoder,
&nVal)) {
On 2015/09/30 20:09:50, Tom Sepez wrote:
> Same thing here.  Can we get rid of the nVal temporary entirely and get
> consistent types?

Done.

[Tom Sepez, 2015-10-01 16:03:48]

lgtm

[Lei Zhang, 2015-10-01 20:16:34]

Committed patchset #15 (id:320001) manually as
ce37d7347f1ab4c9fc1f48a137628da641bb4f14 (presubmit successful).