GNOME Bugzilla – Bug 659114
[core] Some nifty corrections and spellchecking
Last modified: 2012-10-06 11:57:52 UTC
Created attachment 196580 [details] [review] Caps Patch Some possible corrections that I noticed while reading the gstcaps source.
- is this a reverse patch? - please submit a patch in git format-patch format - this is a bunch of random changes with typo fixes, please separate code and typo fixes and preferably separate unrelated code fixes as well, and include a rational for the changes
Created attachment 196664 [details] [review] spellchecks
Created attachment 196666 [details] [review] Changed char to gchar int to gint
Created attachment 196667 [details] [review] GST_IS_CAPS-instead-of-a-simple-NULL-comparison
Created attachment 196668 [details] [review] Is not good to rely on the internals of a structure, better to use accessors and setters
Review of attachment 196664 [details] [review]: ::: gst/gstcaps.c @@ +62,3 @@ * or intersecting. * + * Last reviewed on 2011-9-14 (0.11) I think 0.10.36 would be better @@ +196,3 @@ * Creates a new #GstCaps that is empty. That is, the returned * #GstCaps contains no media formats. + * Caller is responsible for dereferencing the returned caps. It's not dereferencing but "unreferencing", i.e. gst_caps_unref() @@ +243,3 @@ * structure is defined by the arguments, which have the same format * as gst_structure_new(). + * Caller is responsible for dereferencing the returned caps. Same here @@ +336,3 @@ * Converts a #GstStaticCaps to a #GstCaps. * + * Returns: (transfer full): a pointer to the #GstCaps. Dereference after usage. and here
Review of attachment 196666 [details] [review]: ::: gst/gstcaps.c @@ +608,3 @@ if (G_LIKELY (structure)) { GstStructure *structure1; + guint i; This will break the loop a few lines below because it requires signed integers that can go below zero. With unsigned integers the stop condition will never happen. @@ +1733,3 @@ { GstStructure *simplify, *compare, *result = NULL; + guint i, j, start; Same here
Review of attachment 196667 [details] [review]: ::: gst/gstcaps.c @@ +1515,3 @@ /* NULL pointers are no correct GstCaps */ + g_return_val_if_fail (GST_IS_CAPS (caps1), NULL); + g_return_val_if_fail (GST_IS_CAPS (caps2), NULL); Why only in this function and not everywhere else? Also please make the patches against 0.10, we will merge the changes into 0.11 then.
Comment on attachment 196668 [details] [review] Is not good to rely on the internals of a structure, better to use accessors and setters commit 7be3899653ed7dfe31789e657a68735886a1ae15 Author: Fabrizio (Misto) Milo <mistobaan@gmail.com> Date: Thu Sep 15 11:49:43 2011 -0700 caps: use g_value_take_string() and gst_value_get_caps() instead of accessin
I'm tempted to just WONTFIX most of these. Most of them are completely inconsequential bikeshedding/"I think it's prettier/better/nice to" issues. We have barely enough manpower to maintain things at the moment, most of our core devs can only dream of actually having enough time to hack on 0.11, and we really don't need to waste time at the moment by deliberating about char vs. gchar or GST_IS_CAPS vs. caps != NULL (one does a run-time type check btw, and may have performance implications). Not to mention that such changes make it harder to merge stuff in general, with little benefit. Again, IMHO
Most of these are now obsolete or pushed. Let's not do those char -> gchar conversions everywhere. closing.