On Tue, Jul 11, 2017 at 11:24:45AM -0400, David Malcolm wrote: > +/* Some tokens naturally come in pairs e.g.'(' and ')'. > + This class is for tracking such a matching pair of symbols. > + In particular, it tracks the location of the first token, > + so that if the second token is missing, we can highlight the > + location of the first token when notifying the user about the > + problem. */ > + > +template <typename token_pair_traits_t>
the style guide says template arguments should be in mixed case, so TokenPairTraits, and the _t looks odd to my eyes. > +class token_pair > +{ > + private: > + typedef token_pair_traits_t traits_t; I'm not really sure what this is about, you can name it whatever you like as a template argument, and this name seems less descriptive of what its about. > + public: > + /* token_pair's ctor. */ > + token_pair () : m_open_loc (UNKNOWN_LOCATION) {} What do you think of passing the parser to the ctor, and dropping it from the other arguments? It doesn't seem to make sense to support passing in different parsers? > + /* If the next token is the closing symbol for this pair, consume it > + and return it. > + Otherwise, issue an error, highlighting the location of the > + corrsponding opening token, and return NULL. */ typo. > +/* A subclass of token_pair for tracking matching pairs of parentheses. */ > + > +class matching_parens : public token_pair<matching_parens> It seems a little strange for this class to both subclass and be the traits, given that the token_pair class doesn't need objeects of the template argument type. I'd consider writing this as struct matching_paren_traits { static const cpp_ttype open_token_type = CPP_OPEN_PAREN; ... }; typedef token_pair<matching_paren_traits> matching_parens; > +{ > + public: > + static const enum cpp_ttype open_token_type; > + static const enum required_token required_token_open; > + static const enum cpp_ttype close_token_type; > + static const enum required_token required_token_close; Given that these are static consts of integer type I think its fine to define them inline in the class. > +class matching_braces : public token_pair<matching_braces> same comments here. thanks Trev