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 659114 - [core] Some nifty corrections and spellchecking
[core] Some nifty corrections and spellchecking
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
0.11.x
Other Linux
: Normal enhancement
: 0.11.x
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-09-15 02:34 UTC by Fabrizio Milo
Modified: 2012-10-06 11:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Caps Patch (7.66 KB, patch)
2011-09-15 02:34 UTC, Fabrizio Milo
needs-work Details | Review
spellchecks (5.59 KB, patch)
2011-09-15 18:56 UTC, Fabrizio Milo
needs-work Details | Review
Changed char to gchar int to gint (1.38 KB, patch)
2011-09-15 18:59 UTC, Fabrizio Milo
needs-work Details | Review
GST_IS_CAPS-instead-of-a-simple-NULL-comparison (860 bytes, patch)
2011-09-15 18:59 UTC, Fabrizio Milo
needs-work Details | Review
Is not good to rely on the internals of a structure, better to use accessors and setters (876 bytes, patch)
2011-09-15 19:01 UTC, Fabrizio Milo
committed Details | Review

Description Fabrizio Milo 2011-09-15 02:34:26 UTC
Created attachment 196580 [details] [review]
Caps Patch

Some possible corrections that I noticed while reading the gstcaps source.
Comment 1 Tim-Philipp Müller 2011-09-15 09:39:25 UTC
- 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
Comment 2 Fabrizio Milo 2011-09-15 18:56:58 UTC
Created attachment 196664 [details] [review]
spellchecks
Comment 3 Fabrizio Milo 2011-09-15 18:59:06 UTC
Created attachment 196666 [details] [review]
Changed char to gchar int to gint
Comment 4 Fabrizio Milo 2011-09-15 18:59:37 UTC
Created attachment 196667 [details] [review]
GST_IS_CAPS-instead-of-a-simple-NULL-comparison
Comment 5 Fabrizio Milo 2011-09-15 19:01:07 UTC
Created attachment 196668 [details] [review]
Is not good to rely on the internals of a structure, better to use accessors and setters
Comment 6 Sebastian Dröge (slomo) 2011-09-19 07:44:42 UTC
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
Comment 7 Sebastian Dröge (slomo) 2011-09-19 07:46:58 UTC
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
Comment 8 Sebastian Dröge (slomo) 2011-09-19 07:49:48 UTC
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 9 Sebastian Dröge (slomo) 2011-09-19 07:51:07 UTC
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
Comment 10 Tim-Philipp Müller 2011-09-19 11:56:14 UTC
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
Comment 11 Wim Taymans 2012-03-29 13:44:00 UTC
Most of these are now obsolete or pushed. Let's not do those char -> gchar conversions everywhere. closing.