After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 654572 - Set but unused warning
Set but unused warning
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 0.10.23
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-07-13 18:59 UTC by Nicolas Dufresne (ndufresne)
Modified: 2011-07-20 17:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix unused but set (6.13 KB, patch)
2011-07-18 18:56 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2011-07-13 18:59:13 UTC
I came across couple of case of set but unused variable warning preventing compilation of gst-plugins-bad:

http://cgit.collabora.com/git/user/nicolas/gst-plugins-bad.git/log/?h=unused
Comment 1 David Schleef 2011-07-14 01:55:10 UTC
These can all be one commit.

Please just delete code, don't disable it using /* */.  (*Especially* don't use /* */, as that is for comments.  Use #if 0 or #ifdef unused to disable code, that way indent still works correctly.)
Comment 2 Olivier Crête 2011-07-14 02:02:18 UTC
Edward told me exactly the opposite! (to comment out, not remove code)
Comment 3 Edward Hervey 2011-07-14 10:35:48 UTC
Then use #if 0 and not comments. The point is not to lose information/code which is presently unused but would be potentially useful later on.

Also ... it would be good to dig in a bit deeper than "oh, variable unused, let's remove it". There's a chance that it's also the indicator of a bug in the code.

I would have removed all of those a year ago when I did the big clang analysis, but didn't because I wasn't 100% sure they were *really* unused.

In case of doubt, I'd much prefer if you contacted the maintainers/authors of those plugins to make sure those variables are really not needed.
Comment 4 David Schleef 2011-07-14 20:25:55 UTC
I don't mind removing that pretty clearly looks unused.  The code can always be retrieved from git.

Edward is correct that we prefer to use #ifdef unused or #if 0 for larger chunks that are accidentally unused because of a bug -- removing the code papers over the bug.

In any case, this is what code review is for.  If I don't approve of deleting a chunk of code, I'll just remove it from the commit before pushing.
Comment 5 Nicolas Dufresne (ndufresne) 2011-07-14 22:26:51 UTC
That clarify a bit the what to do.

For assrender, ignoring the stride of an image seems wrong, so I'd suggest commenting out with FIXME and a bug id to go with it. The caps in lv2, I think they can simply be removed.

For the timestamp in modplug I just have no idea, thus if 0 might be better. But I don't see obvious reason to file a bug.

In sndfile, there is good chance that the dicount boolean should result in setting discount flag on the created buffer, but again I don't have any context.

Finally the rest seems correct fix to me, like the wildmidi fix where stuff in ifdef had variable declaration outside those ifdef.

So I suggest I squash all this, replace the /* */ with ifdef, file a bug for assrender and set discount flag in sndfile. Those that make sense ?
Comment 6 Nicolas Dufresne (ndufresne) 2011-07-18 18:56:52 UTC
Created attachment 192213 [details] [review]
Fix unused but set
Comment 7 Olivier Crête 2011-07-20 17:12:33 UTC
Committed

commit e8d24859ca5629198c581b570402e9a459839d7c
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Mon Jul 18 14:53:31 2011 -0400

    Fix compilation for unused but not set
    
    https://bugzilla.gnome.org/show_bug.cgi?id=654572