GNOME Bugzilla – Bug 740872
Improve subfilters handling
Last modified: 2014-12-05 17:17:53 UTC
.
Created attachment 291746 [details] [review] filter: Free subfilters
Created attachment 291747 [details] [review] filter: Make subfilters a queue Having subfilters as just two left/right values makes it impossible to have filters with more than 2 subfilters. But it would be very nice if we could have a single filter like: foo=1 AND bar=1 AND baz=0 Usign a queue for the subfilters makes this possible... and it's coming in a future commit.
Created attachment 291748 [details] [review] filter: Add new _full variadic constructors This allows AND-/OR-ing more than 2 subfilters at a time. A unit test is also added.
Created attachment 291749 [details] [review] filter: Add new _fullv constructors This is similar to the ones introduced in the previous commit, but the argument is a NULL-temrinated array of GomFilter, rather than using variadic functions. A unit test is also added.
Created attachment 291766 [details] [review] filter: Free subfilters
Created attachment 291767 [details] [review] filter: Make subfilters a queue Having subfilters as just two left/right values makes it impossible to have filters with more than 2 subfilters. But it would be very nice if we could have a single filter like: foo=1 AND bar=1 AND baz=0 Using a queue for the subfilters makes this possible... and it's coming in a future commit. Conflicts: gom/gom-filter.c
Created attachment 291768 [details] [review] filter: Add new _full variadic constructors This allows AND-/OR-ing more than 2 subfilters at a time. A unit test is also added.
Created attachment 291769 [details] [review] filter: Add new _fullv constructors This is similar to the ones introduced in the previous commit, but the argument is a NULL-temrinated array of GomFilter, rather than using variadic functions. A unit test is also added.
New version of the patches attached, after some comments from slaf on IRC. :)
Review of attachment 291766 [details] [review]: Yes.
Review of attachment 291767 [details] [review]: "Conflicts" in the commit log? You'll need to remove that. ::: gom/gom-filter.c @@ +316,3 @@ + sqls = g_new (gchar *, 1 + g_queue_get_length (priv->subfilters)); + + i = 0; Why not use a for loop? len = g_queue_get_length(queue); for (i = 0; i < len; i++) { f = g_queue_peek_nth (queue, i); ... } @@ +413,3 @@ case GOM_FILTER_OR: + i = 0; + f = g_queue_peek_nth (priv->subfilters, i); Ditto for loop. @@ +461,3 @@ } + if (priv->subfilters != NULL) { No need for braces for one-liner conditionals. @@ +462,3 @@ + if (priv->subfilters != NULL) { + g_queue_free_full (priv->subfilters, clear_filter); g_object_unref()? Is there a case where you would have a null item in the queue, except a bug?
Review of attachment 291768 [details] [review]: Looks fine otherwise. ::: gom/gom-filter.c @@ +274,3 @@ + "mode", GOM_FILTER_AND, + NULL); + filter->priv->subfilters = g_queue_new (); A lot of copy/paste between the 2 functions, would be nice to factor that out.
Review of attachment 291769 [details] [review]: ::: gom/gom-filter.c @@ +310,3 @@ + "mode", GOM_FILTER_AND, + NULL); + filter->priv->subfilters = g_queue_new (); Ditto about the factoring.
Review of attachment 291766 [details] [review]: Pushed.
Created attachment 292080 [details] [review] filter: Make subfilters a queue Having subfilters as just two left/right values makes it impossible to have filters with more than 2 subfilters. But it would be very nice if we could have a single filter like: foo=1 AND bar=1 AND baz=0 Using a queue for the subfilters makes this possible... and it's coming in a future commit.
Created attachment 292081 [details] [review] filter: Add new _full variadic constructors This allows AND-/OR-ing more than 2 subfilters at a time. A unit test is also added.
Created attachment 292082 [details] [review] filter: Add new _fullv constructors This is similar to the ones introduced in the previous commit, but the argument is a NULL-temrinated array of GomFilter, rather than using variadic functions. A unit test is also added.
Created attachment 292083 [details] [review] filter: Factorize some code
Review of attachment 292080 [details] [review]: Looks good apart from the spaces before the brackets.
Review of attachment 292081 [details] [review]: Same comment about the spaces before brackets.
Review of attachment 292082 [details] [review]: Spaces. Brackets.
Review of attachment 292083 [details] [review]: Brackets and spaces, sitting in a tree.
All pushed with space/brackets fixed. Attachment 292080 [details] pushed as 401d40f - filter: Make subfilters a queue Attachment 292081 [details] pushed as fa1f603 - filter: Add new _full variadic constructors Attachment 292082 [details] pushed as 84d9d22 - filter: Add new _fullv constructors Attachment 292083 [details] pushed as 9e26704 - filter: Factorize some code