Coverity DEADCODE - CID 10719

[Product Strategy Coverity Bug Uploader]

This bug is exported from the Coverity Integration Manager on Canonical's servers. For information on how this is done please see this website: https://wiki.ubuntu.com/CanonicalProductStrategy/Coverity
CID: 10719
Checker: DEADCODE
Category: No category available
CWE definition: http://cwe.mitre.org/data/definitions/561.html
File: /tmp/buildd/indicator-sound-0.8.0.0/build/gtk2/src/metadata-menu-item.c
Function: metadata_menuitem_alter_label()
Code snippet:
721 const gchar* _tmp0_;
722 const gchar* _tmp1_;
723 g_return_if_fail (self != NULL);
724 g_return_if_fail (new_title != NULL);
CID 10719 - DEADCODE
After this line (or expression), the value of "_tmp0_" cannot be 0.
725 _tmp0_ = new_title;
CID 10719 - DEADCODE
On this path, the condition "_tmp0_ == NULL" cannot be true.
726 if (_tmp0_ == NULL) {
CID 10719 - DEADCODE
Execution cannot reach this statement "return;".
727 return;
728 }
729 _tmp1_ = new_title;
730 dbusmenu_menuitem_property_set ((DbusmenuMenuitem*) self, DBUSMENU_METADATA_MENUITEM_PLAYER_NAME, _tmp1_);

[Product Strategy Coverity Bug Uploader (coverity-uploader)]

Source file with Coverity annotations.

[Charles Kerr (charlesk)]

I'm still just learning vala so I could be wrong, but I think Coverity is correct.

The vala code that generated the C that Coverity is warning is:

  public void alter_label (string new_title)
  {
    if (new_title == null) return;
    this.property_set (MENUITEM_PLAYER_NAME, new_title);
  }

I think "new_title" isn't allowed to be null because it's not prefixed with '?' in the argument list.

Removing the "if (new_title == null) return;" line removes the C code that generated the Coverity warning.

[Conor Curran (cjcurran)]

Yes I think the signature should be (string? new_title) and then check to see if is NULL. As it stands the check is irrelevant. I would prefer to keep this check in place inorder to protect the UI against rogue dbus updates. Not that i have every witnessed one but it's better off being safe then sorry considering a mplayer's mpris implementation maybe misbehaving (this I have witnessed in the past).

[Charles Kerr (charlesk)]

Sounds good to me. Thanks for reviewing, Conor :)