[Bug 735665] gst-validate should concatenate its issue reporting in some cases.

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=735665
  GStreamer | gst-devtools | git

--- Comment #38 from Thibault Saunier <tsaunier at gnome.org> 2014-10-03 08:19:44 UTC ---
Review of attachment 287630:
 --> (https://bugzilla.gnome.org/review?bug=735665&attachment=287630)

::: validate/gst/validate/gst-validate-report.c
@@ +51,3 @@
 #define GST_CAT_DEFAULT gst_validate_report_debug

+#define GST_VALIDATE_REPORT_SHADOW_REPORTS_LOCK(r)            \

Are you sure we want to dedicated lock names?

@@ +448,3 @@
   if (G_UNLIKELY (g_atomic_int_dec_and_test (&report->refcount))) {
     g_free (report->message);
+    g_list_free_full (report->shadow_reports, (GDestroyNotify)
gst_validate_report_unref);

Missing the lock here.

@@ +628,3 @@
+  gboolean add_shadow_report = TRUE;
+
+  report->master_report = gst_validate_report_ref (master_report);

What happens if we already had one?

You should probably g_return_if_fail(report->master_report);

@@ +655,3 @@
+      12, "", gst_validate_reporter_get_name (report->reporter));
+
+  for (tmp = report->shadow_reports; tmp; tmp = tmp->next) {

Forgot to get the lock here?

--- Comment #39 from Thibault Saunier <tsaunier at gnome.org> 2014-10-03 08:19:45 UTC ---
Review of attachment 287630:
 --> (https://bugzilla.gnome.org/review?bug=735665&attachment=287630)

::: validate/gst/validate/gst-validate-report.c
@@ +51,3 @@
 #define GST_CAT_DEFAULT gst_validate_report_debug

+#define GST_VALIDATE_REPORT_SHADOW_REPORTS_LOCK(r)            \

Are you sure we want to dedicated lock names?

@@ +448,3 @@
   if (G_UNLIKELY (g_atomic_int_dec_and_test (&report->refcount))) {
     g_free (report->message);
+    g_list_free_full (report->shadow_reports, (GDestroyNotify)
gst_validate_report_unref);

Missing the lock here.

@@ +628,3 @@
+  gboolean add_shadow_report = TRUE;
+
+  report->master_report = gst_validate_report_ref (master_report);

What happens if we already had one?

You should probably g_return_if_fail(report->master_report);

@@ +655,3 @@
+      12, "", gst_validate_reporter_get_name (report->reporter));
+
+  for (tmp = report->shadow_reports; tmp; tmp = tmp->next) {

Forgot to get the lock here?

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.