Remove unnecessary fields in top-level parser of Subzero.

Remove unnecessary fields in top-level parser of Subzero.

Cleans up code by removing unnecessary fields/data structures in top-level parser of Subzero. In particular:

1) Uses FunctionDeclarationList.size() instead of NumFunctionIds.

2) Removes the need for vector DefiningFunctionDeclarationList. Instead uses an (incremented) index NextDefiningFunctionID into vector
FunctionDeclarationList.

BUG=None
R=jvoung@chromium.org, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=0c729c8cb60f1316d589a5890abb5bdc1e59bc9f

[Karl, 2015-01-26 22:05:55]

kschimpf@google.com changed reviewers:
+ jvoung@chromium.org, stichnot@chromium.org

[jvoung (off chromium), 2015-01-26 22:58:02]

Could you describe more specifically waas is removed in the commit message
(seems like all related to one TODO)? Otherwise LGTM.

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (left):

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp#oldc...
src/PNaClTranslator.cpp:2915: }
Might be more clear to have a "bool IsProto = Values[2] == 1;" then pass that to
the create().

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:375: unsigned NextDefiningFunctionID;
Might be more clear to put this next FunctionDeclarationList and describe the
relationship.

FunctionDeclarationList is filled first. It's the set of functions (either
defined or isproto).

Then function definitions are encountered/parsed and NextDefiningFunctionID
incremented to track the next actually-defined function.

[Jim Stichnoth, 2015-01-26 23:12:54]

also otherwise lgtm

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:237: unsigned NumDeclaredFunctions =
FunctionDeclarationList.size();
It's probably a good idea to change some of these "unsigned" declarations to
something like size_t (or worse, std::vector::size_type).

[Karl, 2015-01-28 18:58:33]

Committed patchset #2 (id:20001) manually as
0c729c8cb60f1316d589a5890abb5bdc1e59bc9f (presubmit successful).

[Karl, 2015-01-28 18:59:18]

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (left):

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp#oldc...
src/PNaClTranslator.cpp:2915: }
On 2015/01/26 22:58:02, jvoung wrote:
> Might be more clear to have a "bool IsProto = Values[2] == 1;" then pass that
to
> the create().

Done.

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:237: unsigned NumDeclaredFunctions =
FunctionDeclarationList.size();
On 2015/01/26 23:12:54, stichnot wrote:
> It's probably a good idea to change some of these "unsigned" declarations to
> something like size_t (or worse, std::vector::size_type).

For this field, and the local here, I will fix it to size_t. However, I am
hesitant to change more deeply since the base class NaClBitcodeParser assumes
ID's (in general) are unsigned (not size_t_. Will consider a separate CL to
consider more general clean ups on size_t and unsigned.

https://codereview.chromium.org/883493002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:375: unsigned NextDefiningFunctionID;
On 2015/01/26 22:58:02, jvoung wrote:
> Might be more clear to put this next FunctionDeclarationList and describe the
> relationship.
> 
> FunctionDeclarationList is filled first. It's the set of functions (either
> defined or isproto).
> 
> Then function definitions are encountered/parsed and NextDefiningFunctionID
> incremented to track the next actually-defined function.

Done.

[Karl, 2015-02-03 21:19:55]

Patchset #3 (id:40001) has been deleted