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
