Re: [PLUGIN] Fix PLUGIN_FINISH_TYPE

[Romain Geissler <romain dot geissler at gmail dot com>]

Hi,

2011/9/12 Dodji Seketeli <dodji@seketeli.org>:
> Hello Romain,
>
> Romain Geissler <romain.geissler@st.com> a écrit:
>
>> This patch solves some lacks with the PLUGIN_FINISH_TYPE event
>> triggering. For now, both C and C++ parser won't trigger it for enums or
>> for typedef.
>
> AFAICT, typedef declarations are reported by the PLUGIN_FINISH_DECL
> event. ?The predicate is_typedef_decl then returns TRUE when passed the
> decl carried by the event. ?Maybe the plugin's documentation could use
> an addendum for this.

I just checked again, and PLUGIN_FINISH_DECL is triggered for a
typedef in C mode, but not in C++ mode. I'll patch that.
When i recontributed the PLUGIN_FINISH_DECL patch from the original
Brian Hackett, i didn't exactly checked  what may or may not trigger
this new event. I know for example that declaring a function triggers
this event, but defining it does not.

I don't really know when those event should be triggered, we should
clarify the meaning of those.

According to me:
PLUGIN_FINISH_DECL should be triggered anytime the parser parse a
declaration (which includes declaration + definition of function,
typedef definition, namespaces, ..., all DECL_P trees built by the
parser).
For, PLUGIN_FINISH_TYPE i don't really know it means a new type
declaration (or declaration + definition) or if it means usage of a
type (in a function prototype, the type of a local variable. I would
rather vote for new type definition (including typedefs) but for
special cases of struct, you can declare and use them at the same
time:

#include <stdlib.h>

struct my_struct x;
struct my_struct *my_function ();

struct my_struct {
  int field_1;
  int field_2;
};

struct my_struct *my_function () {
>---return NULL;
}

which GCC accepts even with -pedantic.

That's why in this patch i launch a PLUGIN_FINISH_TYPE event every
time the parser meet the "struct" keyword.


So, what the real meaning of all those events ?

>
> [...]
>
>> gcc/cp/
>> 2011-09-12 ?Romain Geissler ?<romain.geissler@gmail.com>
>>
>> ? ? ? * decl.c (grokdeclarator): Trigger PLUGIN_FINISH_TYPE for typedefs.
>
> [...]
>
>> Index: gcc/cp/decl.c
>> ===================================================================
>> --- gcc/cp/decl.c ? ? (revision 178252)
>> +++ gcc/cp/decl.c ? ? (working copy)
>> @@ -9682,6 +9682,9 @@ grokdeclarator (const cp_declarator *dec
>> ? ? ? ? ? ? ? ? ? ? memfn_quals != TYPE_UNQUALIFIED,
>> ? ? ? ? ? ? ? ? ? ? inlinep, friendp, raises != NULL_TREE);
>>
>> + ? ? ?if (TREE_TYPE (decl) != error_mark_node && !DECL_ARTIFICIAL (decl))
>> + ? ? ? ?invoke_plugin_callbacks (PLUGIN_FINISH_TYPE, decl);
>> +
>> ? ? ? ?return decl;
>> ? ? ?}
>>
>
> So I think this change is not necessary.
>
> [...]
>
>
>> Index: gcc/testsuite/gcc.dg/plugin/dumb-plugin-test-1.c
>> ===================================================================
>> --- gcc/testsuite/gcc.dg/plugin/dumb-plugin-test-1.c ?(revision 0)
>> +++ gcc/testsuite/gcc.dg/plugin/dumb-plugin-test-1.c ?(revision 0)
>> @@ -0,0 +1,56 @@
>> +// Test case for the dumb plugin.
>> +// { dg-do compile }
>> +// { dg-options "-O -fplugin-arg-dumb_plugin-ref-pass-name=ccp -fplugin-arg-dumb_plugin-ref-pass-instance-num=1" }
>> +
>
> Here, it would be nice to add a comment saying which plugin acts on this
> plugin's test and what the plugin does (what the output is supposed to
> be). ?That redundant information eases the review, at very least.

Ok

>
> Thanks.
>
> --
> ? ? ? ? ? ? ? ?Dodji
>