GNOME Bugzilla – Bug 727746
Factor buffer flags-to-string code
Last modified: 2014-06-04 11:36:47 UTC
While fixing a strcpy-to-fixed-size-buffer issue found by Coverity, I saw the code that had this was present in three different files. I factored this in gstbuffer.c, but this may warrant comments about the API addition so I'll put it here first. First two patches are what I'd also do to the other two instances of that code should the third patch be NAKed.
Created attachment 273710 [details] [review] change fixed size buffer usage
Created attachment 273711 [details] [review] cleanup
Created attachment 273712 [details] [review] factor two other instances
Comment on attachment 273712 [details] [review] factor two other instances "Since: 1.4", not 1.3.1 :) I think I would prefer to have that in a private header in plugins/elements somewhere instead of public API.
Also a bit iffy because there are more buffer flags one might want to show, but they're media type specific and defined in -base for example.
Would it be less iffy if I added an optional array to the function that maps later flags to custom names ?
Tim, would this change in comment 6 assuage your concern ?
Not really, I think that's even more ugly :) I can live with patch #3, the first two are obsolete then I guess.
Hmm, I'm not sure what's the consensus here. Tim, you agreed with the third patch, but the first two were marked as commit-now (not sure by whom). If I was to push the third patch, the only comment about it is to move it to a private header ?
(In reply to comment #9) > patch, but the first two were marked as commit-now (not sure by whom). Sebastian marked it.
(In reply to comment #9) > Hmm, I'm not sure what's the consensus here. Tim, you agreed with the third > patch, but the first two were marked as commit-now (not sure by whom). If I was > to push the third patch, the only comment about it is to move it to a private > header ? The two first patches are independent afaict and should be pushed. The code duplication makes sense to de-duplicate. Question was how to best do that. The two options afaict are the third patch or a variant thereof as suggested in comment #6. I would prefer to de-duplicate the code but not make it public API for the time being, and if I interpret Sebastian's comment #4 correctly he also prefers that.
Created attachment 277806 [details] [review] factored and private Now private to elements.
Review of attachment 277806 [details] [review]: ::: plugins/elements/gstelements_private.c @@ +24,3 @@ +# include "config.h" +# endif +#endif And why here this #ifndef GST_LICENSE foo? Seems like something wrong with the includes somewhere @@ +67,3 @@ + end++; + } + } I would prefer if this just used GString but otherwise ok :) ::: plugins/elements/gstelements_private.h @@ +28,3 @@ +# include "config.h" +# endif +#endif Why this? Just include config.h in the source file
Created attachment 277862 [details] [review] factored and pivate This include setup was from gst_private.[ch]. Works fine with the GST_LICENCE bit taken out. Do you really want GString use ? I don't really see the point of adding some extra API use for something so simple.
Comment on attachment 277862 [details] [review] factored and pivate I'm fine with not using GString here. But arguably that code is not really simple and has quite some potential for causing crashes and unexpected results if written badly :) I mean you also wouldn't write that in assembler, would you? GString just makes it much easier and keeps all the ugly work out of your way
> wouldn't write that in assembler, would you? I dunno about that... Brings back some happy hacking memories :P But OK, it's not so simple, granted.
Thanks. Pushed. commit a8ca56b42d785e4deaa4d1aa141d4734f8693a66 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Apr 7 14:49:59 2014 +0100 gstbuffer: factor three flags-to-string loops