Name the states for fpdf_parser_parser.

Name the states for fpdf_parser_parser.

Currently the states are numbers, use names instead to make the code
clearer where things are transitioning too.

R=tsepez@chromium.org

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

[dsinclair, 2016-02-18 18:39:04]

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

[dsinclair, 2016-02-18 18:39:04]

PTAL.

I'm explicitly not changing the surrounding code, just the names. This code
needs tests before I feel safe doing structural modifications. This change makes
things a lot clearer when looking at the code I think.

[Tom Sepez, 2016-02-18 19:27:56]

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

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:904: case 13:
Heh.  No 12.

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

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:39: enum class ParserState {
Can we make ParserState an enum nested privately in CPDF_Parser?

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:41: kComment,
Can we keep these in the same order as before so that the values of the states
don't get permuted?  e.g. kWhiteSpace as 1?

I'm worried that in the depths of the logic, they may do some bizzaro thing like
if state > N or some such.

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:43: 
nit: not sure we need blank lines here.

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:607: enum ParserState status
= ParserState::kDefault;
nit: |enum| needed ? Also, can we rename this |state| since "status" means
something slightly different (to me) -- its a "state machine", not a "status
machine". :).

[dsinclair, 2016-02-18 19:47:04]

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

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:904: case 13:
On 2016/02/18 19:27:55, Tom Sepez wrote:
> Heh.  No 12.

Acknowledged.

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

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:39: enum class ParserState {
On 2016/02/18 19:27:55, Tom Sepez wrote:
> Can we make ParserState an enum nested privately in CPDF_Parser?

Done.

Is there a benefit over having it in the anonymous namespace?

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:41: kComment,
On 2016/02/18 19:27:55, Tom Sepez wrote:
> Can we keep these in the same order as before so that the values of the states
> don't get permuted?  e.g. kWhiteSpace as 1?
> 
> I'm worried that in the depths of the logic, they may do some bizzaro thing
like
> if state > N or some such.


I checked through the code, all uses of status are to assign the new state or in
the switch(status). 

Putting in the existing order will force us to put in a dummy 12 state, which we
don't need.

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:43: 
On 2016/02/18 19:27:55, Tom Sepez wrote:
> nit: not sure we need blank lines here.

Done. Was an attempt at grouping and readability.

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:607: enum ParserState status
= ParserState::kDefault;
On 2016/02/18 19:27:55, Tom Sepez wrote:
> nit: |enum| needed ? Also, can we rename this |state| since "status" means
> something slightly different (to me) -- its a "state machine", not a "status
> machine". :).

Done.

[Tom Sepez, 2016-02-18 19:49:17]

lgtm

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

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:39: enum class ParserState {
On 2016/02/18 19:47:04, dsinclair wrote:
> On 2016/02/18 19:27:55, Tom Sepez wrote:
> > Can we make ParserState an enum nested privately in CPDF_Parser?
> 
> Done.
> 
> Is there a benefit over having it in the anonymous namespace?
Yes, it tells the reader that this state is associated with the parser class and
only the parser class.

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:41: kComment,
On 2016/02/18 19:47:04, dsinclair wrote:
> On 2016/02/18 19:27:55, Tom Sepez wrote:
> > Can we keep these in the same order as before so that the values of the
states
> > don't get permuted?  e.g. kWhiteSpace as 1?
> > 
> > I'm worried that in the depths of the logic, they may do some bizzaro thing
> like
> > if state > N or some such.
> 
> 
> I checked through the code, all uses of status are to assign the new state or
in
> the switch(status). 
> 
> Putting in the existing order will force us to put in a dummy 12 state, which
we
> don't need.

Acknowledged.

[dsinclair, 2016-02-18 19:51:03]

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

https://codereview.chromium.org/1715483002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:39: enum class ParserState {
On 2016/02/18 19:49:16, Tom Sepez wrote:
> On 2016/02/18 19:47:04, dsinclair wrote:
> > On 2016/02/18 19:27:55, Tom Sepez wrote:
> > > Can we make ParserState an enum nested privately in CPDF_Parser?
> > 
> > Done.
> > 
> > Is there a benefit over having it in the anonymous namespace?
> Yes, it tells the reader that this state is associated with the parser class
and
> only the parser class.
> 
> 


Ah, right. Too spoiled by one class per file. Wasn't thinking about the
many-classes per file we have.

[dsinclair, 2016-02-18 19:55:49]

Description was changed from

==========
Name the states for fpdf_parser_parser.

Currently the states are numbers, use names instead to make the code
clearer where things are transitioning too.
==========

to

==========
Name the states for fpdf_parser_parser.

Currently the states are numbers, use names instead to make the code
clearer where things are transitioning too.

R=tsepez@chromium.org

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

[dsinclair, 2016-02-18 19:55:50]

Committed patchset #3 (id:40001) manually as
c5ac0202004cd38071a9f6f0d7f3e5d8669ccfbb (presubmit successful).