On Wed, 2017-07-12 at 09:13 -0400, Trevor Saunders wrote:
> 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.
My thinking here was to have a way to refer to the template arg
in other locations, but, yes, it turns out not to be necessary.
> > +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.
Fixed by renaming the arg to just "traits_t".
> > + 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?
I'm in two minds about this:
(a) yes: the parser ptr is always going to unchanged, and could be
passed in to the ctor, which would avoid needing to pass it in
everywhere the token_pair<T> instancd is used
(b) is the optimizer good enough to realize this, and avoid storing
a second copy of the parser ptr on the stack? (presumably to
optimize away the 2nd copy of the parser ptr, every call to methods
of the class would need to be inlined, and then the two on-stack
member fields be split up into individual fields, and then copy
propagation could eliminate it).
In this version of the kit I opted to keep passing in the parser ptr
at all the usage sites, but I'm open to making it a field of the
class.
I'm hoping for input on this from the C/C++ frontend maintainers.
> > + /* 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.
Thanks; fixed (in both C and C++ frontends).
> > +/* 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;
Done (for both C and C++ frontends).
> > +{
> > + 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.
Done, though in the C frontend there are some const char * static
consts which have to be defined outside (since constexpr is guaranteed
to be available to us).
> > +class matching_braces : public token_pair<matching_braces>
>
> same comments here.
>
> thanks
>
> Trev
Thanks.
Updated patch kit follows.
Successfully bootstrapped®rtested the combination of the patches
on x86_64-pc-linux-gnu (as before I've split up the patches for
ease-of-review; they're not independent of each other).
Patch 1 is already approved; are patches 2 and 3 OK for trunk?
Thanks
Dave
David Malcolm (3):
matching tokens: c-family parts
Matching tokens: C parts (v2)
matching tokens: C++ parts (v2)
gcc/c-family/c-common.c | 17 +-
gcc/c-family/c-common.h | 3 +-
gcc/c/c-parser.c | 644 ++++++++++------
gcc/c/c-parser.h | 8 +-
gcc/cp/parser.c | 811 +++++++++++++--------
gcc/testsuite/c-c++-common/missing-close-symbol.c | 33 +
gcc/testsuite/c-c++-common/missing-symbol.c | 50 ++
.../g++.dg/diagnostic/unclosed-extern-c.C | 3 +
.../g++.dg/diagnostic/unclosed-function.C | 3 +
.../g++.dg/diagnostic/unclosed-namespace.C | 2 +
gcc/testsuite/g++.dg/diagnostic/unclosed-struct.C | 3 +
gcc/testsuite/g++.dg/parse/pragma2.C | 4 +-
gcc/testsuite/gcc.dg/unclosed-init.c | 3 +
13 files changed, 1071 insertions(+), 513 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/missing-close-symbol.c
create mode 100644 gcc/testsuite/c-c++-common/missing-symbol.c
create mode 100644 gcc/testsuite/g++.dg/diagnostic/unclosed-extern-c.C
create mode 100644 gcc/testsuite/g++.dg/diagnostic/unclosed-function.C
create mode 100644 gcc/testsuite/g++.dg/diagnostic/unclosed-namespace.C
create mode 100644 gcc/testsuite/g++.dg/diagnostic/unclosed-struct.C
create mode 100644 gcc/testsuite/gcc.dg/unclosed-init.c
--
1.8.5.3
