mate-settings-plugin-info: unused parameter 'settings'

[rbuj]

mate-settings-plugin-info.c: In function 'plugin_enabled_cb':
mate-settings-plugin-info.c:269:44: warning: unused parameter 'settings' [-Wunused-parameter]
  269 | plugin_enabled_cb (GSettings              *settings,
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

Test: disable/enable msd plugin

$ gsettings get org.mate.SettingsDaemon.plugins.housekeeping active
true
$ gsettings set org.mate.SettingsDaemon.plugins.housekeeping active false

[rbuj]

It fixes an incorrect use of the macro G_GNUC_UNUSED, that was introduced in 7e8357d5cc6e04ae96734e2c8e1f87b9a154a52c

[sc0w]

It fixes an incorrect use of the macro G_GNUC_UNUSED, that was introduced in 7e8357d

you are wrong, https://github.com/mate-desktop/mate-settings-daemon/commit/7e8357d5cc6e04ae96734e2c8e1f87b9a154a52c mark unused parameters as unused in signals, you are using this unused parameter with this change, LGTM

[raveit65]

It fixes an incorrect use of the macro G_GNUC_UNUSED, that was introduced in 7e8357d

you are wrong, 7e8357d mark unused parameters as unused in signals, you are using this unused parameter with this change, LGTM

??? 0_o Why merging if using G_GNUC_UNUSED is correct? Can you enlighten me please?

[sc0w]

mate-settings-plugin-info.c: In function 'plugin_enabled_cb':
mate-settings-plugin-info.c:269:44: warning: unused parameter 'settings' [-Wunused-parameter]
  269 | plugin_enabled_cb (GSettings              *settings,
      |                    ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

UNUSED, it needs more explanations?

without this PR is unused, with this PR is used

[rbuj]

Yes I agree, it is a serious mistake on the part of @sc0w: not checking whether or not the instance to connect to is used.

        g_signal_connect (G_OBJECT (info->priv->settings), "changed::active",
                          G_CALLBACK (plugin_enabled_cb), info);

[muktupavels]

Serious mistake? Before your MR parameter was unused and he correctly marked it as such!

[rbuj]

Ok, I'm not an expert to find the similarity between the two lines below (info->priv->settings). Do you think the best solution is to say that the variable is not used?

if (g_settings_get_boolean (info->priv->settings, key))

g_signal_connect (G_OBJECT (info->priv->settings), "changed::active",
                  G_CALLBACK (plugin_enabled_cb), info);

[rbuj]

Here is another example where the developer has not worked hard to find the best proposal or solution #334

[raveit65]

UNUSED, it needs more explanations?

without this PR is unused, with this PR is used

I only asked because i missed any logic in previous post.

[muktupavels]

@rbuj sounds like you don't understand -Wunused-parameter...

[rbuj]

@muktupavels I don't think so, but you are free to think whatever you want :)

[muktupavels]

static void
plugin_enabled_cb (GSettings              *settings,
                   gchar                  *key,
                   MateSettingsPluginInfo *info)
{
        if (g_settings_get_boolean (info->priv->settings, key)) {
                mate_settings_plugin_info_activate (info);
        } else {
                mate_settings_plugin_info_deactivate (info);
        }
}

I do not see where parameter settings is used here. I guess I am blind...

[raveit65]

Serious mistake? Before your MR parameter was unused and he correctly marked it as such!

Confused, that means it is wrong to use a parameter as fix for a warning? And better mark them all as unused? I am in doubt that this was the intention of the author of those warning.

[rbuj]

less than a week ago the same reviewer warned me of a similar issue, using userdata->path->to->variable instead of using variable when it's available as a parameter of the function.

Edit: well, in fact I think he didn't explain the warning or how to fix it, he just told me that a new warning appeared on the build -Wunused-parameter

[muktupavels]

Serious mistake? Before your MR parameter was unused and he correctly marked it as such!

Confused, that means it is wrong to use a parameter as fix for a warning?

No, no... Before any changes to that function you got -Wunused-parameter warning because settings parameter was not used. To fix/remove warning you have 3 options:

• disable -Wunused-parameter warnings
• mark unused parameter as unused
• start using parameter

And better mark them all as unused?

You inspect code and make decision what to do. Both changes was/are correct.

I am in doubt that this was the intention of the author of those warning.

One thing I dont get is if @rubj still got warning after G_GNUC_UNUSED was added. Looking at pull request his output shows version without G_GNUC_UNUSED... And commit message does not explain anything.

[sc0w]

@rbuj

good to see, thanks to https://github.com/mate-desktop/mate-settings-daemon/commit/7e8357d5cc6e04ae96734e2c8e1f87b9a154a52c you can use some unused parameters with little changes

my workflow in the future will be the same, I like to work with little changes and other people can use unused parameters later

[rbuj]

@muktupavels

grep -rl G_GNUC_UNUSED . | xargs sed -i 's/G_GNUC_UNUSED//g'

[rbuj]

@sc0w nice, I just updated my build script https://gist.github.com/rbuj/bc62164be10a7290f5213c47711c7998

[rbuj]

@muktupavels similar MR which start to use unused function parameters

• https://github.com/mate-desktop/mate-utils/pull/300
• https://github.com/mate-desktop/mate-applets/pull/524