Modified Token class to store various types of data. Added debug code to dump token to an output ...

Modified Token class to store various types of data. Added debug code to dump token to an output stream.
Committed: http://code.google.com/p/angleproject/source/detail?r=737

[kbr1, 2011-08-19 20:15:51]

LGTM overall. Couple of comments.

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/To...
File src/compiler/preprocessor/new/Token.h (right):

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/To...
src/compiler/preprocessor/new/Token.h:39: std::string* sval;
It seems unusual to use std::string* instead of just std::string; are you trying
to avoid string copying?

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/pp.y
File src/compiler/preprocessor/new/pp.y (right):

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/pp...
src/compiler/preprocessor/new/pp.y:41: std::vector<std::string*>* slist;
It seems to me the memory management of this type might be difficult, since you
have to explicitly delete each element as well as the vector.

[Alok Priyadarshi, 2011-08-19 21:39:48]

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/To...
File src/compiler/preprocessor/new/Token.h (right):

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/To...
src/compiler/preprocessor/new/Token.h:39: std::string* sval;
On 2011/08/19 20:15:51, kbr1 wrote:
> It seems unusual to use std::string* instead of just std::string; are you
trying
> to avoid string copying?

This is because putting a std::string (class with non-trivial constructor) in a
union is not allowed. You can only have POD types.

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/pp.y
File src/compiler/preprocessor/new/pp.y (right):

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/pp...
src/compiler/preprocessor/new/pp.y:41: std::vector<std::string*>* slist;
On 2011/08/19 20:15:51, kbr1 wrote:
> It seems to me the memory management of this type might be difficult, since
you
> have to explicitly delete each element as well as the vector.

Yeah it is unfortunate. The same is true with pp::TokenVector which is a vector
of token pointers. I think memory management will be a pain but not too
difficult, because everything gets reduced to one rule where we can delete all
allocated memory. I will also add plenty of memory tests.

[Alok Priyadarshi, 2011-08-22 08:16:51]

Daniel/Ken: I committed a portion of this patch in r737 because I wanted to work
on it during the weekend. If you have any suggestions, please let me know and I
will address them in a new patch.

[dgkoch, 2011-08-22 19:43:41]

LGTM

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/pp.y
File src/compiler/preprocessor/new/pp.y (right):

http://codereview.appspot.com/4920041/diff/1/src/compiler/preprocessor/new/pp...
src/compiler/preprocessor/new/pp.y:41: std::vector<std::string*>* slist;
On 2011/08/19 21:39:48, Alok Priyadarshi wrote:
> On 2011/08/19 20:15:51, kbr1 wrote:
> > It seems to me the memory management of this type might be difficult, since
> you
> > have to explicitly delete each element as well as the vector.
> 
> Yeah it is unfortunate. The same is true with pp::TokenVector which is a
vector
> of token pointers. I think memory management will be a pain but not too
> difficult, because everything gets reduced to one rule where we can delete all
> allocated memory. I will also add plenty of memory tests.

Please do make sure that allocation failures are handled and can be relatively
easily propagated out as well.  This is one of the failure points of the old
preprocessor...