[1/7] Fix handling of ident-less declarations
diff mbox

Message ID E1LYJZx-0001ls-Nn@ZenIV.linux.org.uk
State Mainlined, archived
Headers show

Commit Message

Al Viro Feb. 14, 2009, 12:25 p.m. UTC
The rule for ident-less declaration is
	declaration -> declaration-specifiers ;
not
	declaration -> declaration-specifiers abstract-declarator;

IOW, struct foo; is OK and so's struct foo {int x; int y;} (and even
simply int; is allowed by syntax - it's rejected by constraints, but
that's a separate story), but not struct foo (void); and its ilk.

See C99 6.7p1 for syntax; C90 is the same in that area and gcc also
behaves the same way.  Unlike gcc I've made it a warning (gcc produces
a hard error for those).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 parse.c                    |   10 +++++++++-
 validation/missing-ident.c |   18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletions(-)
 create mode 100644 validation/missing-ident.c

Comments

Christopher Li Feb. 14, 2009, 6:45 p.m. UTC | #1
On Sat, Feb 14, 2009 at 4:25 AM, Al Viro <viro@ftp.linux.org.uk> wrote:
>
> The rule for ident-less declaration is

wow, great.

The series applies to the tree fine. First impression of the series
looks fine. I am still working on it.

Thanks for the patches. BTW, this is not the "lazy type evaluation"
you are talking about right?

Chris

>        declaration -> declaration-specifiers ;
> not
>        declaration -> declaration-specifiers abstract-declarator;
>
> IOW, struct foo; is OK and so's struct foo {int x; int y;} (and even
> simply int; is allowed by syntax - it's rejected by constraints, but
> that's a separate story), but not struct foo (void); and its ilk.
>
> See C99 6.7p1 for syntax; C90 is the same in that area and gcc also
> behaves the same way.  Unlike gcc I've made it a warning (gcc produces
> a hard error for those).
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  parse.c                    |   10 +++++++++-
>  validation/missing-ident.c |   18 ++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletions(-)
>  create mode 100644 validation/missing-ident.c
>
> diff --git a/parse.c b/parse.c
> index 73e7b65..6100fc2 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -2265,14 +2265,22 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis
>        token = declaration_specifiers(token, &ctype, 0);
>        decl = alloc_symbol(token->pos, SYM_NODE);
>        decl->ctype = ctype;
> +       /* Just a type declaration? */
> +       if (match_op(token, ';')) {
> +               apply_modifiers(token->pos, &decl->ctype);
> +               return token->next;
> +       }
> +
>        token = declarator(token, decl, &ident, 0);
>        apply_modifiers(token->pos, &decl->ctype);
>
>        decl->endpos = token->pos;
>
>        /* Just a type declaration? */
> -       if (!ident)
> +       if (!ident) {
> +               warning(token->pos, "missing identifier in declaration");
>                return expect(token, ';', "end of type declaration");
> +       }
>
>        /* type define declaration? */
>        is_typedef = (ctype.modifiers & MOD_TYPEDEF) != 0;
> diff --git a/validation/missing-ident.c b/validation/missing-ident.c
> new file mode 100644
> index 0000000..ce73983
> --- /dev/null
> +++ b/validation/missing-ident.c
> @@ -0,0 +1,18 @@
> +int [2];
> +int *;
> +int (*);
> +int ();
> +int;
> +struct foo;
> +union bar {int x; int y;};
> +struct baz {int x, :3, y:2;};
> +/*
> + * check-name: handling of identifier-less declarations
> + *
> + * check-error-start
> +missing-ident.c:1:8: warning: missing identifier in declaration
> +missing-ident.c:2:6: warning: missing identifier in declaration
> +missing-ident.c:3:8: warning: missing identifier in declaration
> +missing-ident.c:4:7: warning: missing identifier in declaration
> + * check-error-end
> + */
> --
> 1.5.6.6
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Feb. 14, 2009, 7:08 p.m. UTC | #2
On Sat, Feb 14, 2009 at 10:45:41AM -0800, Christopher Li wrote:
> On Sat, Feb 14, 2009 at 4:25 AM, Al Viro <viro@ftp.linux.org.uk> wrote:
> >
> > The rule for ident-less declaration is
> 
> wow, great.
> 
> The series applies to the tree fine. First impression of the series
> looks fine. I am still working on it.
> 
> Thanks for the patches. BTW, this is not the "lazy type evaluation"
> you are talking about right?

No, just the first batch of declaration parser fixes...  The next group is
about separating ctype-as-part-of-symbol from ctype-as-parser-state uses.
After that - getting declaration_specifiers() to sane shape, which, BTW,
will relieve the situation with mode bits.  Then -  cleaning up the type
handling...
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Feb. 14, 2009, 8:31 p.m. UTC | #3
On Sat, Feb 14, 2009 at 07:08:36PM +0000, Al Viro wrote:
> On Sat, Feb 14, 2009 at 10:45:41AM -0800, Christopher Li wrote:
> > On Sat, Feb 14, 2009 at 4:25 AM, Al Viro <viro@ftp.linux.org.uk> wrote:
> > >
> > > The rule for ident-less declaration is
> > 
> > wow, great.
> > 
> > The series applies to the tree fine. First impression of the series
> > looks fine. I am still working on it.
> > 
> > Thanks for the patches. BTW, this is not the "lazy type evaluation"
> > you are talking about right?
> 
> No, just the first batch of declaration parser fixes...  The next group is
> about separating ctype-as-part-of-symbol from ctype-as-parser-state uses.
> After that - getting declaration_specifiers() to sane shape, which, BTW,
> will relieve the situation with mode bits.  Then -  cleaning up the type
> handling...

BTW, I wonder whether if it would be better to just scan for the end
of attributes after ( when we are deciding whether it's a function or
nested declarator, leaving the actual handling of these guys to after
the decision.  The thing is, unlike gcc we have the token list anyway,
and it's easier to not bother with passing that crap to parameter_type_list(),
etc.

I'll try to do that and see what falls out; potentially that replaces
7/7 in this series.
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Feb. 14, 2009, 9:30 p.m. UTC | #4
On Sat, Feb 14, 2009 at 08:31:54PM +0000, Al Viro wrote:
> On Sat, Feb 14, 2009 at 07:08:36PM +0000, Al Viro wrote:
> > On Sat, Feb 14, 2009 at 10:45:41AM -0800, Christopher Li wrote:
> > > On Sat, Feb 14, 2009 at 4:25 AM, Al Viro <viro@ftp.linux.org.uk> wrote:
> > > >
> > > > The rule for ident-less declaration is
> > > 
> > > wow, great.
> > > 
> > > The series applies to the tree fine. First impression of the series
> > > looks fine. I am still working on it.
> > > 
> > > Thanks for the patches. BTW, this is not the "lazy type evaluation"
> > > you are talking about right?
> > 
> > No, just the first batch of declaration parser fixes...  The next group is
> > about separating ctype-as-part-of-symbol from ctype-as-parser-state uses.
> > After that - getting declaration_specifiers() to sane shape, which, BTW,
> > will relieve the situation with mode bits.  Then -  cleaning up the type
> > handling...
> 
> BTW, I wonder whether if it would be better to just scan for the end
> of attributes after ( when we are deciding whether it's a function or
> nested declarator, leaving the actual handling of these guys to after
> the decision.  The thing is, unlike gcc we have the token list anyway,
> and it's easier to not bother with passing that crap to parameter_type_list(),
> etc.
> 
> I'll try to do that and see what falls out; potentially that replaces
> 7/7 in this series.

... no, it doesn't ;-/  Too much PITA with error recovery, so the original
version still stands.  Alas.
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christopher Li Feb. 16, 2009, 11:14 a.m. UTC | #5
On Sun, Feb 15, 2009 at 5:42 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Feb 15, 2009 at 04:33:16PM -0800, Christopher Li wrote:
>> All applied and pushed. I update some of the validations error because
>> of the tab width change.
>
> BTW, I'm not sure that this messing with tabstops is a good idea.
> Note that tokenizer is _still_ the hottest part of the entire
> thing, so we need to be damn careful around it.  You are getting the
> slow path of nextchar() on \t now, which can add up to a lot of
> extra overhead...

How about the attached patch?

I move white space handling out side of the nextchar_slow().
Let nextchar_slow() only handle two case: possible EOF and '\\'.

Chris

Patch
diff mbox

diff --git a/parse.c b/parse.c
index 73e7b65..6100fc2 100644
--- a/parse.c
+++ b/parse.c
@@ -2265,14 +2265,22 @@  struct token *external_declaration(struct token *token, struct symbol_list **lis
 	token = declaration_specifiers(token, &ctype, 0);
 	decl = alloc_symbol(token->pos, SYM_NODE);
 	decl->ctype = ctype;
+	/* Just a type declaration? */
+	if (match_op(token, ';')) {
+		apply_modifiers(token->pos, &decl->ctype);
+		return token->next;
+	}
+
 	token = declarator(token, decl, &ident, 0);
 	apply_modifiers(token->pos, &decl->ctype);
 
 	decl->endpos = token->pos;
 
 	/* Just a type declaration? */
-	if (!ident)
+	if (!ident) {
+		warning(token->pos, "missing identifier in declaration");
 		return expect(token, ';', "end of type declaration");
+	}
 
 	/* type define declaration? */
 	is_typedef = (ctype.modifiers & MOD_TYPEDEF) != 0;
diff --git a/validation/missing-ident.c b/validation/missing-ident.c
new file mode 100644
index 0000000..ce73983
--- /dev/null
+++ b/validation/missing-ident.c
@@ -0,0 +1,18 @@ 
+int [2];
+int *;
+int (*);
+int ();
+int;
+struct foo;
+union bar {int x; int y;};
+struct baz {int x, :3, y:2;};
+/*
+ * check-name: handling of identifier-less declarations
+ *
+ * check-error-start
+missing-ident.c:1:8: warning: missing identifier in declaration
+missing-ident.c:2:6: warning: missing identifier in declaration
+missing-ident.c:3:8: warning: missing identifier in declaration
+missing-ident.c:4:7: warning: missing identifier in declaration
+ * check-error-end
+ */