Re: [patch] plugin directory / common location for plugins

[Matthias Klose <doko at ubuntu dot com>]

On 06.04.2010 03:45, Le-Chun Wu wrote:
> Sorry for being late in the game. Here are my comments and thoughts:
>
> While some of us don't really feel the need to create a new GCC policy for a
> default plugin location, I actually agree with Basile and Matthias that
> having such a default location surely will make it easier for the plugin
> writers to ship their plugins and for plugin users to use them. So I would
> support this change and approve the patch with the following comments:

> > --- a/src/gcc/testsuite/gcc.dg/plugin/plugin.exp (revision
> > +++ b/src/gcc/testsuite/gcc.dg/plugin/plugin.exp (working
> > @@ -51,6 +51,7 @@
> >      { ggcplug.c ggcplug-test-1.c } \
> >       { one_time_plugin.c one_time-test-1.c } \
> >       { start_unit_plugin.c start_unit-test-1.c } \
> > +    { plugdir_plugin.c plugdir-test-1.c } \
>
> It's not clear to me what the purpose of this test is. It's apparently not
> testing the short plugin name (and default location) support as the plugin
> is still being called in the standard way. It seems to test the function
> "plugin_directory_name", but you don't appear to check for the correctness
> of the returned name. I think better test cases should be created/added.

removed, and added four new patches checking the passing of the options and the 
expansion in cc1.

> > +++ b/src/gcc/gcc-plugin.h (working
> > @@ -141,4 +141,10 @@
> >
> >   extern int unregister_callback (const char *plugin_name, int event);
> >
> > +
> > +/* Retrieve the plugin directory name, as returned by the
> > +   -fprint-file-name=plugin argument to the gcc program, which is the
> > +   -iplugindir program argument to cc1.  */
> > +extern const char* plugin_directory_name (void);
>
> I would change the name of this function to something like
> "default_plugin_dir_name" to make it clearer what it does.

done.

> > +++ b/src/gcc/common.opt (working
> > @@ -1535,6 +1535,11 @@
> >   Common JoinedOrMissing Negative(gcoff)
> >   Generate debug information in extended XCOFF format
> >
> > +
> > +iplugindir
> > +Common Joined Separate Var(plugindir_string)
> > +-iplugindir<dir>  Set<dir>  to be the plugin directory
> > +
>
> Add an entry in doc/invoke.texi? Especially you want to mention the this
> flag is not meant for the end users.

I did set the Undocumented property instead (alternatively attached is another 
patch to document the option).

> > @@ -818,3 +844,12 @@
> >   {
> >     return event_last;
> >   }
> > +
> > +/* Retrieve the plugin directory.
>
> s/Retrieve the plugin directory/Retrieve the default plugin directory

done.

> Given that this patch is actually a new feature (and in my opinion not
> critical), I am not sure it's suitable for 4.5. (I will leave it to release
> manager and others.)

Lets move it to the trunk first. I still would like to see this for 4.5, if 
possible. Same reason for the -fld=gold|ld.bfd to avoid handling with path names 
which differ on systems/installations.

Please find attached the updated patch.

  Matthias

Attachment: plugindir.diff
Description: Text document

Attachment: plugindir-doc.diff
Description: Text document