improve syntax errors
Hello!
I have used GCC for decades and would like to contribute a little. :-)
I would like to see if I can improve the syntax errors.
Here is one example code:
int x = 3) + 0;
I have created this ugly experimental patch:
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -2228,7 +2228,10 @@ c_parser_declaration_or_fndef (c_parser
*parser, bool fndef_ok,
}
else
{
- c_parser_error (parser, "expected %<,%> or %<;%>");
+ if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN))
+ c_parser_error (parser, "extraneous )");
+ else
+ c_parser_error (parser, "expected %<,%> or %<;%>");
c_parser_skip_to_end_of_block_or_statement (parser);
return;
}
Before my patch:
test1.c:3:12: error: expected ‘,’ or ‘;’ before ‘)’ token
After my patch:
test1.c:3:12: error: extraneous ) before ‘)’ token
That is not perfect neither.
I have 2 questions..
* can somebody give me a hint how I improve the error message so it
does not say "before ) token"?
* how do I run the tests?
Basically I want that when there is a unmatched extra ) or } or ] then
it should just say "extraneous .." instead of "expected ',' or ';'.
Adding a ',' or ';' in the example code will not fix the syntax error.
Best regards,
Daniel Marjamäki
Re: syntax errors
Thank you for the quick reply.
> how about "stray %qs token"?
I will change.
> I wonder how much we want to special-case this. Are you thinking about
> the case where there's a stray symbol in the code (perhaps due to a
> stray keypress, or unfinished manual edits)?
At the moment I only think about ) } ]
I would like to focus on only those 3 to start with. but it sounds
good to prepare for more stray tokens later.
> if (c_parser_next_token_is_meaningless_p (parser))
> complain_about_stray_token (parser);
> else
> ...
sure!
> It might make sense to emit a fix-it hint suggesting the removal of the
> stray token.
It is 50% chance that the closing paranthesis should be removed. Maybe
there is a missing "(".
Maybe the error message should indicate that.. something like "either
there is missing "(" or this ")" is a stray token".
Best regards,
Daniel Marjamäki
Improve syntax error
Hello!
I just chose the word "unmatched" for now.. let me know if "stray" or
some other word is better. :-)
I have a work in progress patch that as far as I can tell works.
I wonder if you see some problems with my design. What do I need to
do..? Basically I have chosen to add a nesting_depth to the c_parser
struct. That is updated in the consume_token. I first tried to add
such variable in c_parser_declaration_or_fndef() but that was much
more difficult.
I wanted to warn about unmatched }. It is however more complicated
than I thought. For two reasons:
The first reason is the hard problem, but maybe we can ignore this now also:
void f()
{
} // <- looking at the indentation, it seems preferable to warn about this
}
The second reason is easier.. I just don't want to get into too deep
water too quickly. To warn about that also I can't just have a
nesting_depth anymore. I need to have a stack that track the nesting.
I have experimented with a linked list like this and made it work:
struct nesting {
cpp_ttype type;
struct nesting *next;
};
but well here I run into memory management and I also wonder if you
like the design decision to put a linked list in the c_parser struct.
Here is my current patch...
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 972b629c092..eabc5ffa15e 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -171,6 +171,8 @@ struct GTY(()) c_parser {
/* How many look-ahead tokens are available (0 - 4, or
more if parsing from pre-lexed tokens). */
unsigned int tokens_avail;
+ /* nesting depth in expression (parentheses / squares) */
+ unsigned int nesting_depth;
/* True if a syntax error is being recovered from; false otherwise.
c_parser_error sets this flag. It should clear this flag when
enough tokens have been consumed to recover from the error. */
@@ -763,6 +765,22 @@ c_parser_next_tokens_start_declaration (c_parser *parser)
return false;
}
+/* Nesting start token */
+
+static bool c_parser_is_nesting_start (c_parser *parser)
+{
+return c_parser_next_token_is (parser, CPP_OPEN_PAREN) ||
+ c_parser_next_token_is (parser, CPP_OPEN_SQUARE);
+}
+
+/* Nesting end token */
+
+static bool c_parser_is_nesting_end (c_parser *parser)
+{
+return c_parser_next_token_is (parser, CPP_CLOSE_PAREN) ||
+ c_parser_next_token_is (parser, CPP_CLOSE_SQUARE);
+}
+
/* Consume the next token from PARSER. */
void
@@ -772,6 +790,10 @@ c_parser_consume_token (c_parser *parser)
gcc_assert (parser->tokens[0].type != CPP_EOF);
gcc_assert (!parser->in_pragma || parser->tokens[0].type != CPP_PRAGMA_EOL);
gcc_assert (parser->error || parser->tokens[0].type != CPP_PRAGMA);
+ if (c_parser_is_nesting_start (parser))
+parser->nesting_depth++;
+ else if (parser->nesting_depth > 0 && c_parser_is_nesting_end (parser))
+parser->nesting_depth--;
parser->last_token_location = parser->tokens[0].location;
if (parser->tokens != &parser->tokens_buf[0])
parser->tokens++;
@@ -1673,6 +1695,20 @@ add_debug_begin_stmt (location_t loc)
add_stmt (stmt);
}
+static bool c_parser_unmatched_p (c_parser *parser)
+{
+ if (c_parser_is_nesting_end (parser))
+return parser->nesting_depth == 0;
+ return false;
+}
+
+static void complain_about_unmatched_token (c_parser *parser)
+{
+ c_token *token = c_parser_peek_token(parser);
+ error_at (token->location, "unmatched %<%s%>",
+cpp_type2name(token->type, token->flags));
+}
+
/* Parse a declaration or function definition (C90 6.5, 6.7.1, C99
6.7, 6.9.1, C11 6.7, 6.9.1). If FNDEF_OK is true, a function definition
is accepted; otherwise (old-style parameter declarations) only other
@@ -2228,7 +2264,10 @@ c_parser_declaration_or_fndef (c_parser
*parser, bool fndef_ok,
}
else
{
- c_parser_error (parser, "expected %<,%> or %<;%>");
+ if (c_parser_unmatched_p (parser))
+complain_about_unmatched_token (parser);
+ else
+c_parser_error (parser, "expected %<,%> or %<;%>");
c_parser_skip_to_end_of_block_or_statement (parser);
return;
}
diff --git a/gcc/testsuite/gcc.dg/unmatched.c b/gcc/testsuite/gcc.dg/unmatched.c
new file mode 100644
index 000..bd458a01109
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/unmatched.c
@@ -0,0 +1,19 @@
+
+/* { dg-do compile } */
+
+void f1() {
+ int a = 0)+3; /* { dg-error "unmatched" } */
+}
+
+void f2() {
+ int b = (1]+3; /* { dg-error "expected" } */
+}
+
+void f3() {
+ int b = 1]+3; /* { dg-error "unmatched" } */
+}
+
+void f4() {
+ int c = (1))+3; /* { dg-error "unmatched" } */
+}
+
Best regards,
Daniel Marjamäki
Re: Improve syntax error
Thanks!
I will take care of the indentation and fix the comment.
> I think the indentation warnings should catch that?
I get this:
void f()
{
}
} // <- error: expected identifier or '(' before '}' token
I ran with -Wall -Wextra -pedantic and did not see a indentation
warning. Am I missing some indentation warning? The error message I
get is a little misplaced. I think it's fine to warn about that } but
it could also say in the error message that the problem is probably
the previous }
> Should this say something like "expected ) or , or ;"?
No none of those suggestions will solve the error.
Look at this code:
int x = 3) + 0;
Writing a ) or , or ; will not fix the syntax error. You have to
remove the ) or add a ( somewhere.
Den lör 5 jan. 2019 kl 09:50 skrev Segher Boessenkool
:
>
> Hi Daniel,
>
> Some mostly boring comments:
>
> On Fri, Jan 04, 2019 at 09:25:10PM +0100, Daniel Marjamäki wrote:
> > The first reason is the hard problem, but maybe we can ignore this now also:
> >
> > void f()
> > {
> > } // <- looking at the indentation, it seems preferable to warn about
> > this
> > }
>
> I think the indentation warnings should catch that?
>
> > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> > index 972b629c092..eabc5ffa15e 100644
> > --- a/gcc/c/c-parser.c
> > +++ b/gcc/c/c-parser.c
> > @@ -171,6 +171,8 @@ struct GTY(()) c_parser {
> >/* How many look-ahead tokens are available (0 - 4, or
> > more if parsing from pre-lexed tokens). */
> >unsigned int tokens_avail;
> > + /* nesting depth in expression (parentheses / squares) */
>
> Start sentences with a capital, and end with full stop space space. I
> realise this isn't a full sentence, but the comment right above does this
> as well ;-)
>
> > @@ -763,6 +765,22 @@ c_parser_next_tokens_start_declaration (c_parser
> > *parser)
> >return false;
> > }
> >
> > +/* Nesting start token */
> > +
> > +static bool c_parser_is_nesting_start (c_parser *parser)
> > +{
> > +return c_parser_next_token_is (parser, CPP_OPEN_PAREN) ||
> > + c_parser_next_token_is (parser, CPP_OPEN_SQUARE);
>
> Indents should use tabs for every leading eight spaces.
>
> > @@ -2228,7 +2264,10 @@ c_parser_declaration_or_fndef (c_parser
> > *parser, bool fndef_ok,
> > }
> >else
> > {
> > - c_parser_error (parser, "expected %<,%> or %<;%>");
> > + if (c_parser_unmatched_p (parser))
> > +complain_about_unmatched_token (parser);
>
> Should this say something like "expected ) or , or ;"?
>
> > + else
> > +c_parser_error (parser, "expected %<,%> or %<;%>");
> >c_parser_skip_to_end_of_block_or_statement (parser);
> >return;
> > }
>
>
> Segher
Re: Improve syntax error
Here is a new patch with fixed comments and indentation
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 972b629c092..294ff34fe55 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -171,6 +171,8 @@ struct GTY(()) c_parser {
/* How many look-ahead tokens are available (0 - 4, or
more if parsing from pre-lexed tokens). */
unsigned int tokens_avail;
+ /* Nesting depth in expression (parentheses / squares). */
+ unsigned int nesting_depth;
/* True if a syntax error is being recovered from; false otherwise.
c_parser_error sets this flag. It should clear this flag when
enough tokens have been consumed to recover from the error. */
@@ -763,6 +765,22 @@ c_parser_next_tokens_start_declaration (c_parser *parser)
return false;
}
+/* Nesting start token. */
+
+static bool c_parser_is_nesting_start (c_parser *parser)
+{
+ return c_parser_next_token_is (parser, CPP_OPEN_PAREN) ||
+ c_parser_next_token_is (parser, CPP_OPEN_SQUARE);
+}
+
+/* Nesting end token. */
+
+static bool c_parser_is_nesting_end (c_parser *parser)
+{
+ return c_parser_next_token_is (parser, CPP_CLOSE_PAREN) ||
+ c_parser_next_token_is (parser, CPP_CLOSE_SQUARE);
+}
+
/* Consume the next token from PARSER. */
void
@@ -772,6 +790,10 @@ c_parser_consume_token (c_parser *parser)
gcc_assert (parser->tokens[0].type != CPP_EOF);
gcc_assert (!parser->in_pragma || parser->tokens[0].type != CPP_PRAGMA_EOL);
gcc_assert (parser->error || parser->tokens[0].type != CPP_PRAGMA);
+ if (c_parser_is_nesting_start (parser))
+parser->nesting_depth++;
+ else if (parser->nesting_depth > 0 && c_parser_is_nesting_end (parser))
+parser->nesting_depth--;
parser->last_token_location = parser->tokens[0].location;
if (parser->tokens != &parser->tokens_buf[0])
parser->tokens++;
@@ -1673,6 +1695,20 @@ add_debug_begin_stmt (location_t loc)
add_stmt (stmt);
}
+static bool c_parser_unmatched_p (c_parser *parser)
+{
+ if (c_parser_is_nesting_end (parser))
+return parser->nesting_depth == 0;
+ return false;
+}
+
+static void complain_about_unmatched_token (c_parser *parser)
+{
+ c_token *token = c_parser_peek_token (parser);
+ error_at (token->location, "unmatched %qs",
+cpp_type2name (token->type, token->flags));
+}
+
/* Parse a declaration or function definition (C90 6.5, 6.7.1, C99
6.7, 6.9.1, C11 6.7, 6.9.1). If FNDEF_OK is true, a function definition
is accepted; otherwise (old-style parameter declarations) only other
@@ -2228,7 +2264,10 @@ c_parser_declaration_or_fndef (c_parser
*parser, bool fndef_ok,
}
else
{
- c_parser_error (parser, "expected %<,%> or %<;%>");
+ if (c_parser_unmatched_p (parser))
+complain_about_unmatched_token (parser);
+ else
+c_parser_error (parser, "expected %<,%> or %<;%>");
c_parser_skip_to_end_of_block_or_statement (parser);
return;
}
diff --git a/gcc/testsuite/gcc.dg/unmatched.c b/gcc/testsuite/gcc.dg/unmatched.c
new file mode 100644
index 000..bd458a01109
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/unmatched.c
@@ -0,0 +1,19 @@
+
+/* { dg-do compile } */
+
+void f1() {
+ int a = 0)+3; /* { dg-error "unmatched" } */
+}
+
+void f2() {
+ int b = (1]+3; /* { dg-error "expected" } */
+}
+
+void f3() {
+ int b = 1]+3; /* { dg-error "unmatched" } */
+}
+
+void f4() {
+ int c = (1))+3; /* { dg-error "unmatched" } */
+}
+
Den lör 5 jan. 2019 kl 18:02 skrev Daniel Marjamäki
:
>
> Thanks!
>
> I will take care of the indentation and fix the comment.
>
> > I think the indentation warnings should catch that?
>
> I get this:
>
> void f()
> {
> }
> } // <- error: expected identifier or '(' before '}' token
>
> I ran with -Wall -Wextra -pedantic and did not see a indentation
> warning. Am I missing some indentation warning? The error message I
> get is a little misplaced. I think it's fine to warn about that } but
> it could also say in the error message that the problem is probably
> the previous }
>
> > Should this say something like "expected ) or , or ;"?
>
> No none of those suggestions will solve the error.
>
> Look at this code:
>
> int x = 3) + 0;
>
> Writing a ) or , or ; will not fix the syntax error. You have to
> remove the ) or add a ( somewhere.
>
>
>
> Den lör 5 jan. 2019 kl 09:50 skrev Segher Boessenkool
> :
> >
> > Hi Daniel,
> >
> > Some mostly boring comments:
> >
> > On Fri, Jan 04, 2019 at 09:25:10PM +0100, Daniel Marjamäki wrote:
> > > The first reason is the hard problem, but maybe we can ignore this now
> > > also:
> > >
>
Re: Improve syntax error
Ping
Den lör 5 jan. 2019 kl 20:44 skrev Daniel Marjamäki
:
>
> Here is a new patch with fixed comments and indentation
>
> diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> index 972b629c092..294ff34fe55 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -171,6 +171,8 @@ struct GTY(()) c_parser {
>/* How many look-ahead tokens are available (0 - 4, or
> more if parsing from pre-lexed tokens). */
>unsigned int tokens_avail;
> + /* Nesting depth in expression (parentheses / squares). */
> + unsigned int nesting_depth;
>/* True if a syntax error is being recovered from; false otherwise.
> c_parser_error sets this flag. It should clear this flag when
> enough tokens have been consumed to recover from the error. */
> @@ -763,6 +765,22 @@ c_parser_next_tokens_start_declaration (c_parser *parser)
>return false;
> }
>
> +/* Nesting start token. */
> +
> +static bool c_parser_is_nesting_start (c_parser *parser)
> +{
> + return c_parser_next_token_is (parser, CPP_OPEN_PAREN) ||
> + c_parser_next_token_is (parser, CPP_OPEN_SQUARE);
> +}
> +
> +/* Nesting end token. */
> +
> +static bool c_parser_is_nesting_end (c_parser *parser)
> +{
> + return c_parser_next_token_is (parser, CPP_CLOSE_PAREN) ||
> + c_parser_next_token_is (parser, CPP_CLOSE_SQUARE);
> +}
> +
> /* Consume the next token from PARSER. */
>
> void
> @@ -772,6 +790,10 @@ c_parser_consume_token (c_parser *parser)
>gcc_assert (parser->tokens[0].type != CPP_EOF);
>gcc_assert (!parser->in_pragma || parser->tokens[0].type !=
> CPP_PRAGMA_EOL);
>gcc_assert (parser->error || parser->tokens[0].type != CPP_PRAGMA);
> + if (c_parser_is_nesting_start (parser))
> +parser->nesting_depth++;
> + else if (parser->nesting_depth > 0 && c_parser_is_nesting_end (parser))
> +parser->nesting_depth--;
>parser->last_token_location = parser->tokens[0].location;
>if (parser->tokens != &parser->tokens_buf[0])
> parser->tokens++;
> @@ -1673,6 +1695,20 @@ add_debug_begin_stmt (location_t loc)
>add_stmt (stmt);
> }
>
> +static bool c_parser_unmatched_p (c_parser *parser)
> +{
> + if (c_parser_is_nesting_end (parser))
> +return parser->nesting_depth == 0;
> + return false;
> +}
> +
> +static void complain_about_unmatched_token (c_parser *parser)
> +{
> + c_token *token = c_parser_peek_token (parser);
> + error_at (token->location, "unmatched %qs",
> +cpp_type2name (token->type, token->flags));
> +}
> +
> /* Parse a declaration or function definition (C90 6.5, 6.7.1, C99
> 6.7, 6.9.1, C11 6.7, 6.9.1). If FNDEF_OK is true, a function definition
> is accepted; otherwise (old-style parameter declarations) only other
> @@ -2228,7 +2264,10 @@ c_parser_declaration_or_fndef (c_parser
> *parser, bool fndef_ok,
> }
>else
> {
> - c_parser_error (parser, "expected %<,%> or %<;%>");
> + if (c_parser_unmatched_p (parser))
> +complain_about_unmatched_token (parser);
> + else
> +c_parser_error (parser, "expected %<,%> or %<;%>");
>c_parser_skip_to_end_of_block_or_statement (parser);
>return;
> }
> diff --git a/gcc/testsuite/gcc.dg/unmatched.c
> b/gcc/testsuite/gcc.dg/unmatched.c
> new file mode 100644
> index 000..bd458a01109
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/unmatched.c
> @@ -0,0 +1,19 @@
> +
> +/* { dg-do compile } */
> +
> +void f1() {
> + int a = 0)+3; /* { dg-error "unmatched" } */
> +}
> +
> +void f2() {
> + int b = (1]+3; /* { dg-error "expected" } */
> +}
> +
> +void f3() {
> + int b = 1]+3; /* { dg-error "unmatched" } */
> +}
> +
> +void f4() {
> + int c = (1))+3; /* { dg-error "unmatched" } */
> +}
> +
>
> Den lör 5 jan. 2019 kl 18:02 skrev Daniel Marjamäki
> :
> >
> > Thanks!
> >
> > I will take care of the indentation and fix the comment.
> >
> > > I think the indentation warnings should catch that?
> >
> > I get this:
> >
> > void f()
> > {
> > }
> > } // <- error: expected identifier or '(' before '}' token
> >
> > I ran with -Wall -Wextra -pedantic and did not see a indentation
> > warning. Am I missing some indentation warning? The error message I
> > get is a little misplaced. I think it's fine to warn about that } but
> > it could also say in the error message that the
sample plugins
Hello! In my humle opinion it would be useful with some sample plugins. For example on one of these pages: http://gcc.gnu.org/wiki/plugins http://gcc.gnu.org/onlinedocs/gccint/Plugins.html I have created two sample plugins that are available here: http://www.github.com/danmar/gcc-plugins Feel free to add these sample plugins. Feedback is also very much welcome. Regards, Daniel
Re: MELT plugin: test fopen
Hello Pierre! It is an interesting plugin. I'll keep an eye on your repository. Thanks! Daniel Marjamäki 2011/2/9 Pierre Vittet : > Hello, > > I would like to present you a small plugin, which could be a good exemple of > a MELT use case. > This plugin allows to monitor that after every call to the fopen function, > we have a test on the pointer returned by fopen (monitoring that it is not > null). > > It creates a pass after SSA and works on gimple. It firstly matchs the > gimple_call on fopen and save the tree corresponding to the FILE *. Then we > check that the next instruction is a gimple_cond between the saved tree and > the NULL ptr. > When no test are found, the following error is returned: > "test_fopen_cfile.c:35:11: warning: fopen not followed by a test on his > returned pointer [enabled by default]" > > I think MELT is particulary adaptated when we have to match tree or gimples > like I do here. > > For the moment I have only used it on small test file. I will try to see > what it gives on a more realistic small to medium application. > > The code can be find on github: https://github.com/Piervit/GMWarn . The idea > is to add more plugins. If you have some ideas or remarks, I am interested > (however I still have to learn a lot from both GCC and MELT). > > If you try the code you will need to use the GCC MELT branch (or there is > some changes to do in the Makefile to have it with MELT as plugin). > > > Regards! > >
patch: don't issue -Wreorder warnings when order doesn't matter
Hello!
In my humble opinion the -Wreorder has noise. When the order doesn't
matter I would prefer that warnings are not issued.
In this email I include a patch that I would like to get comments
about. The patch will suppress warnings if all members are initialized
with constant values.
I am not very good at GCC internals so I wonder if I made some serious
mistake when using TREE_VALUE, TREE_CODE, etc? Perhaps I overlook
something?
Here is sample code that currently generates warnings but my patch
suppress those warnings:
class Fred {
private:
int a;
int b;
public:
Fred() : b(0), a(0) { }
};
I think the next step will be to suppress the warning if both the
members that the message is about are initialized with constant
values.
Best regards,
Daniel
Index: gcc/cp/init.c
===
--- gcc/cp/init.c (revision 176862)
+++ gcc/cp/init.c (working copy)
@@ -711,6 +711,7 @@
VEC(tree,gc) *vbases;
int i;
int uses_unions_p;
+ int all_inits_are_const; /* all members are initialized with a
constant value */
/* Build up a list of initializations. The TREE_PURPOSE of entry
will be the subobject (a FIELD_DECL or BINFO) to initialize. The
@@ -741,6 +742,25 @@
without issuing a warning. */
next_subobject = sorted_inits;
+ all_inits_are_const = 1;
+ if (warn_reorder)
+{
+ for (init = mem_inits; init; init = TREE_CHAIN (init))
+{
+ tree tree_value;
+
+ tree_value = TREE_VALUE(init);
+ if (TREE_CODE(tree_value) == TREE_LIST)
+tree_value = TREE_VALUE(tree_value);
+
+ if (TREE_CODE(tree_value) != INTEGER_CST)
+{
+ all_inits_are_const = 0;
+ break;
+}
+}
+}
+
/* Go through the explicit initializers, filling in TREE_PURPOSE in
the SORTED_INITS. */
for (init = mem_inits; init; init = TREE_CHAIN (init))
@@ -762,7 +782,7 @@
/* Issue a warning if the explicit initializer order does not
match that which will actually occur.
??? Are all these on the correct lines? */
- if (warn_reorder && !subobject_init)
+ if (warn_reorder && !all_inits_are_const && !subobject_init)
{
if (TREE_CODE (TREE_PURPOSE (next_subobject)) == FIELD_DECL)
warning (OPT_Wreorder, "%q+D will be initialized after",
Re: Re: patch: don't issue -Wreorder warnings when order doesn't matter
Hello! > Why doesn't it matter in this case but it matters when the initializer > are non-constant? It doesn't matter because the program will behave the same no matter if the initializations are reordered or not. Logically it will behave just as the user expects no matter if he expects reordering or not. When one initializer is non-constant there might be a dependency between the two initializations and the wrong order might be a bug. I would like to silence such warnings also, but I don't want to try to determine if there is dependencies or not. > If you don't want to fix up your code, just compile it with -Wno-reorder. I don't want to use -Wno-reorder , because then I won't see the real problems. Don't get me wrong, I like this check. When gcc generates noise I think it is better to fix gcc than to fix my code. The only reason I can think of to keep this noise is for purely stylistic reasons. Somebody might think it is a stylistic problem to initialize members backwards. But then -Wreorder should also warn about common assignments and I doubt anybody wants that. Best regards, Daniel
Re: Re: patch: don't issue -Wreorder warnings when order doesn't matter
2011/7/29 Richard Guenther :
> 2011/7/29 Daniel Marjamäki :
>> Hello!
>>
>>> Why doesn't it matter in this case but it matters when the initializer
>>> are non-constant?
>>
>> It doesn't matter because the program will behave the same no matter
>> if the initializations are reordered or not. Logically it will behave
>> just as the user expects no matter if he expects reordering or not.
>>
>> When one initializer is non-constant there might be a dependency
>> between the two initializations and the wrong order might be a bug.
>> I would like to silence such warnings also, but I don't want to try to
>> determine if there is dependencies or not.
>>
>>
>>> If you don't want to fix up your code, just compile it with -Wno-reorder.
>>
>> I don't want to use -Wno-reorder , because then I won't see the real
>> problems. Don't get me wrong, I like this check.
>>
>>
>> When gcc generates noise I think it is better to fix gcc than to fix my code.
>>
>> The only reason I can think of to keep this noise is for purely
>> stylistic reasons. Somebody might think it is a stylistic problem to
>> initialize members backwards. But then -Wreorder should also warn
>> about common assignments and I doubt anybody wants that.
>
> Ok, I think the idea of the patch sounds reasonable then (but maybe
> we can do even better for more cases by adjusting the
>
> + if (TREE_CODE(tree_value) != INTEGER_CST)
>
> check to, say, if (! TREE_CONSTANT (tree_value)) which at least
> would also cover floating-point constants and strings. Eventually
> C++0x offers some more useful predicate give its constexpr feature.
>
> I suppose C++ maintainers will have to have a look here anyway.
>
> And I guess the example in the manual should be adjusted as Jakub
> says.
>
> Thanks,
> Richard.
>
>> Best regards,
>> Daniel
>>
>
Hello!
Thanks for your tip. I adjusted the patch, using TREE_CONSTANT. It works.
I also agree about updating the example in the manual.
Best regards,
Daniel
Index: gcc/cp/init.c
===
--- gcc/cp/init.c (revision 176925)
+++ gcc/cp/init.c (working copy)
@@ -711,6 +711,7 @@
VEC(tree,gc) *vbases;
int i;
int uses_unions_p;
+ int all_inits_are_const; /* all members are initialized with a
constant value */
/* Build up a list of initializations. The TREE_PURPOSE of entry
will be the subobject (a FIELD_DECL or BINFO) to initialize. The
@@ -741,6 +742,26 @@
without issuing a warning. */
next_subobject = sorted_inits;
+ /* check if all explicit initializations use constant values */
+ all_inits_are_const = 1;
+ if (warn_reorder)
+{
+ for (init = mem_inits; init; init = TREE_CHAIN (init))
+{
+ tree tree_value;
+
+ tree_value = TREE_VALUE(init);
+ if (TREE_CODE(tree_value) == TREE_LIST)
+tree_value = TREE_VALUE(tree_value);
+
+ if (! TREE_CONSTANT (tree_value))
+{
+ all_inits_are_const = 0;
+ break;
+}
+}
+}
+
/* Go through the explicit initializers, filling in TREE_PURPOSE in
the SORTED_INITS. */
for (init = mem_inits; init; init = TREE_CHAIN (init))
@@ -762,7 +783,7 @@
/* Issue a warning if the explicit initializer order does not
match that which will actually occur.
??? Are all these on the correct lines? */
- if (warn_reorder && !subobject_init)
+ if (warn_reorder && !all_inits_are_const && !subobject_init)
{
if (TREE_CODE (TREE_PURPOSE (next_subobject)) == FIELD_DECL)
warning (OPT_Wreorder, "%q+D will be initialized after",
Re: Re: patch: don't issue -Wreorder warnings when order doesn't matter
2011/7/30 Joern Rennecke : > Quoting Jonathan Wakely : > >> I would object to changing the behaviour, or if it changes then it >> should be controllable so I can continue to get the current behaviour, >> e.g. -Wreorder=0 does what you propose, -Wreorder=1 does what we have >> now, and -Wreorder is equivalent to -Wreorder=1 > > That sounds somewhat obscure (e.g. why isn't -Wreorder=0 the same > as -Wno-reorder), and at some point people might demand negative values > for more discriminating checks and floating point values for in-between > choices. > > I think more descriptive would be: > -Wreorder=nonconst and -Wreorder=any > > If someone miraculously cheats Rice's theorem, or wants to propose to get > as close as possible to tell if reordering has a semantic effect as is > feasible to tell in a compiler, you could call it -Wreorder=relevant or > somesuch. > > Thanks. In my humble opinion the -Wreorder=nonconst and -Wreorder=any sounds ok. > I want to know the mem-initializers are in the wrong order ASAP so I can > correct them immediately, not when I change the initializer to a non-constant. ok I understand. Best regards, Daniel
Re: Re: patch: don't issue -Wreorder warnings when order doesn't matter
> > If the initializer is constant, but the member value that's being > initialized has a > non-trivial constructor with a side effect, your patch will inhibit the > warning > but the program will not behave the same as if reordering had not happened. > > Peter > Yes. It sounds unlikely. But not impossible of course. I could also make sure the member variables are POD types before I inhibit the warning. I just have no idea how I check if a member is POD. But I could investigate this. I think, this will still remove most of the -Wreorder warnings that I get. Best regards, Daniel
Re: Re: patch: don't issue -Wreorder warnings when order doesn't matter
> What if the object being constructed has only POD-type members with constant > initializers but is declared volatile I don't understand really... but it doesn't matter, I give up.
Plugin that parse tree
Hello!
I want to write a plugin that parse the AST. Could I get some hint
about how to do it?
I have been reading this excellent article:
http://codesynthesis.com/~boris/blog/2010/05/10/parsing-cxx-with-gcc-plugin-part-2/
The problem is that I fail to follow those advices.
I fail to use 'global_namespace':
daniel@daniel:~/gcc/build/gcc$ ./xgcc -fplugin=./myplugin2.so -c test1.c
cc1: error: cannot load plugin ./myplugin2.so
./myplugin2.so: undefined symbol: global_namespace
Here is the code:
#include "gcc-plugin.h"
#include "cp/cp-tree.h"
#include
int plugin_is_GPL_compatible;
static void traverse(tree ns)
{
}
void override_gate(void *gcc_data, void *user_data)
{
printf("myplugin1:override_gate\n");
// Process AST
traverse(global_namespace);
}
int
plugin_init (struct plugin_name_args *plugin_info,
struct plugin_gcc_version *version)
{
printf("myplugin1\n");
register_callback(plugin_info->base_name,
PLUGIN_OVERRIDE_GATE,
&override_gate,
0);
return 0;
}
Best regards,
Daniel Marjamäki
Re: Plugin that parse tree
GCC-MELT is an interesting project. But it seems to be very difficult
to write lisp scripts. You don't have a C interface also, do you?
I would like to see how I can use plain C.
Regards,
Daniel
2011/1/23 Basile Starynkevitch :
> On Sun, 23 Jan 2011 11:58:21 +0100
> Daniel Marjamäki wrote:
>
>> Hello!
>>
>> I want to write a plugin that parse the AST. Could I get some hint
>> about how to do it?
>
>
> You could use GCC MELT for that purpose. See www.gcc-melt.org
>
> Regards
> --
> Basile STARYNKEVITCH http://starynkevitch.net/Basile/
> email: basilestarynkevitchnet mobile: +33 6 8501 2359
> 8, rue de la Faiencerie, 92340 Bourg La Reine, France
> *** opinions {are only mine, sont seulement les miennes} ***
>
Re: Plugin that parse tree
Hello! > Either change your test file to .cpp, or add "-x c++" to the command-line. that worked. thank you. I don't want to limit my plugin to C++. But to start with it is ok. > The major issue is to understand all the details of GCC internal > representations (i.e. Trees, Gimples). Did you understand them? I don't understand anything about the internal representations yet. I don't even know which representation I am currently parsing. I want to get started. If you have any advice it is welcome. I have read the "Gcc internals" manual. It is well written but it doesn't have much example code. Do you know about any simple example code? Regards, Daniel 2011/1/23 Dave Korn : > On 23/01/2011 10:58, Daniel Marjamäki wrote: > >> I fail to use 'global_namespace': >> daniel@daniel:~/gcc/build/gcc$ ./xgcc -fplugin=./myplugin2.so -c test1.c >> cc1: error: cannot load plugin ./myplugin2.so > > You're running the C compiler (cc1) here, not the C++ one (cc1plus), because > you've passed a file with the plain .c extension to the driver. > >> ./myplugin2.so: undefined symbol: global_namespace > > So it doesn't have this global variable that only exists in cc1plus. > > Either change your test file to .cpp, or add "-x c++" to the command-line. > > In general, as long as your plugin refers to global_namespace directly, it's > not going to be compatible with any of the other language sub-compilers apart > from cc1plus. If you wanted it to work with any kind of language, I think > you'd need to look up global_namespace using dlsym (and handle the case when > it was not found), rather than linking against it directly. > > cheers, > DaveK > >
Re: Plugin that parse tree
Do you have any opinion about adding a warning for:
int f(char c)
{
return 10 * (c == 13) ? 1 : 2;
}
The multiplication has no effect. The function returns either 1 or 2.
It would be interesting to know how a MELT script could look like for
such a case.
As far as I see the multiplication doesn't exist in the gimple format
(looking at a.c.004t.gimple generated by -fdump-tree-all).
Re: Plugin that parse tree
2011/1/24 Ian Lance Taylor : > The problem with warnings for this kind of code in C/C++ is that it > often arises in macro expansions. I see... so it won't be included in gcc. :-( It was my goal to get it into GCC. But I still think it's an interesting idea that I'll look into. Regards, Daniel
Re: Plugin that parse tree
2011/1/24 Ian Lance Taylor : > Daniel Marjamäki writes: > >> 2011/1/24 Ian Lance Taylor : >> >>> The problem with warnings for this kind of code in C/C++ is that it >>> often arises in macro expansions. >> >> I see... so it won't be included in gcc. :-( > > Actually, I think it could be included in gcc, provided you (or > somebody) first implements a way to not warn in a macro expansion. > > Ian > I first thought it sounded too difficult. Even though conceptually in my mind it seems to be a pretty simple task. I assume the preprocessor emits tokens in some structure format. When a token is emitted it should be pretty simple to check if a macro is expanded, shouldn't it? So I just add some flag to this token structure. And then I need to preserve that information at least until "constant folding" occurs. I haven't even located the "constant folding" yet. I will try to investigate this. Regards, Daniel
Re: Plugin that parse tree
I see. Good. 2011/1/27 Tom Tromey : >> "Ian" == Ian Lance Taylor writes: > > Ian> The problem with warnings for this kind of code in C/C++ is that it > Ian> often arises in macro expansions. I think it would be necessary to > Ian> first develop a scheme which lets us determine whether code resulted > Ian> from a macro expansion or not, which I think would be quite useful in a > Ian> number of different cases. > > There is a patch series pending for this. > > See the thread "Tracking locations of tokens resulting from macro > expansion". > > Tom >
