Add a new macro for exporting the native plugin symbols and use it in the loaders

Review Request #578 — Created March 22, 2021 and submitted

grim
gplugin/gplugin
default
GPLUGIN-132
9d58376975c0
pidgin

Add a new macro for exporting the native plugin symbols and use it in the loaders

Compiled and ran the unit tests

Description From Last Updated

Some parts of this review appear to be missing. The macros are not defined anywhere.

QuLogicQuLogic

These should define the names of the pre-arranged arguments to the function (or accept args that define the names.)

QuLogicQuLogic

Change "It should be ..." to "This function should be ..." to match GPLUGIN_(UN)LOAD.

QuLogicQuLogic

a unique

QuLogicQuLogic

a unique

QuLogicQuLogic

a unique

QuLogicQuLogic
grim
grim
QuLogic
  1. 
      
  2. Some parts of this review appear to be missing. The macros are not defined anywhere.

    1. so... I was trying to do this in phases to make it easier to review, and posted the second phase instead of the first.. whoops..

  3. 
      
grim
QuLogic
  1. 
      
  2. gplugin/gplugin-native-plugin.c (Diff revision 4)
     
     

    These should define the names of the pre-arranged arguments to the function (or accept args that define the names.)

    1. Ah, to be clearer, I only meant it needed to define in the documentation what the names were, but if you preferred re-writing it like this as well, that's fine. Are you okay with it now, or prefer to do like GObject and mandate a name of something like <name>_{query,load,unload}?

    2. So I decided to change it because I was never a fan of the first version where you had to get the plugin name correct for each of the macros. I knew it was error prone but thought it'd be easier to deal with, but then when you mentioned the parameter hiding it exposed the bigger of of throwing compiler warnings for unused parameters. For example, most plugins never need the GError in the query function, the loaders never use the plugin parameter in their unload function, etc. So we would be forcing people to have compiler warnings which sucks..

      Also we can't just use the macro on the name, because in the future if something is being remaped for static something like the following example, we can't swap out the G_MODULE_EXPORT.

      G_MODULE_EXPORT GPluginPluginInfo *GPLUGIN_QUERY(foo)(G_GNUC_UNUSED GError **error)
      

      TL;DR: Allowing the user to control their functions and forcing them to match a, now public, function pointer type, gives them more
      control over their build while still giving up the pre-processor magic to be able to compile it statically.

    3. No, I didn't mean a macro wrapped around the name, but like G_DEFINE_OBJECT which expects <name>_init, <Name>Class, etc. So GPLUGIN_NATIVE_PLUGIN_DECLARE(name) expects <name>_query, <name>_load, <name>_unload or similar.

    4. That wouldn't fix the over all problem though where we need to rename the function to something unique for a static build and have a G_MODULE_EXPORT'd function with a known name for the loader to find when loading dynamically because the loader isn't going to know that it needs to look for <name>_query, <name>_load, and <name>_unload.

    5. Why would that be a problem? You'd be renaming the gplugin_query in the macro, not <name>_query. This is similar to G_DEFINE_TYPE which uses <name>_init even though that's not what it actually puts in the vtable.

    6. The <name>_init goes into the VTable for GTypeInfo, but anyways...

      I think I originally misunderstood your question. Are you suggesting something like this which would avoid passing the names into the macro?

      static GPluginInfo *
      test_foo_query(G_GNUC_UNUSED GError **error) {
          return NULL;
      }
      
      static gboolean
      test_foo_load(G_GNUC_UNUSED GPluginPlugin *plugin, G_GNUC_UNUSED GError **error) {
          return FALSE;
      }
      
      static gboolean
      test_foo_unload(G_GNUC_UNUSED GPluginPlugin *plugin, G_GNUC_UNUSED GError **error) {
          return FALSE;
      }
      
      GPLUGIN_NATIVE_PLUGIN_DECLARE(test_foo)
      
    7. Right yea, that's what I meant; so similar to how the GLib macros assume names.

  3. gplugin/gplugin-native-plugin.c (Diff revision 4)
     
     
     

    Change "It should be ..." to "This function should be ..." to match GPLUGIN_(UN)LOAD.

  4. gplugin/reference/native-plugins.xml (Diff revision 4)
     
     

    a unique

  5. gplugin/reference/native-plugins.xml (Diff revision 4)
     
     

    a unique

  6. gplugin/reference/native-plugins.xml (Diff revision 4)
     
     

    a unique

  7. 
      
grim
grim
grim
QuLogic
  1. Ship It!
  2. 
      
grim
Review request changed

Status: Closed (submitted)

Loading...