GNOME Bugzilla – Bug 685016
GLib.List<G> implementing BidirList<G> and List<G>
Last modified: 2016-04-07 22:01:50 UTC
I see GXml project implementing most Gee Collection interfaces, like Traversable, Iterable and an Iterator, to get access to Gee collection concepts but relaying a GLib.List<G>, that interesting implementation keeps GLib.List API, and I get the idea to create a new class for the same but using just Gee API.
Created attachment 225312 [details] [review] New Gee.GList class This is the first implemenation of GLib.List<G> as Gee Collection called Gee.GList. Test Cases fail for now but if approved in Gee I can continue fixing them.
Review of attachment 225312 [details] [review]: > public override G remove_at (int index) { > unowned GLib.List<G> l = list.nth(index); > G d = l.data; Here should be transfer of ownership I presume. > list.remove (l); > nelements = -1; I believe it should be -= 1 (there is no need to recalculate size). > return d; Again - transfer of ownership. > } -------------- > public override List<G>? slice (int start, int stop) { > var l = new GLib.List<G>(); Why are you creating a list of size 1? Shouldn't you set l to null? > for (int i = stop; i > start; i--) { What if stop > l.length? > G e = list.nth(start + i).data; i already contains start so I presume it shouldn't have start + i but i - 1. It looks like it is working in O(n^2). > l.prepend(e); > } > var ln = new GList<G> (); > ln.list = l; We could pass size while we at it. > return ln; > } -------------- > public override bool add (G item) { > list.prepend (item); > nelements = -1; > return list != null ? true : false; > } We don't need to recalculate size. In addition add for all other lists adds to the end (yes - I know it is O(N) in GList)... -------------- > /** > * {@inheritDoc} > */ > public override void clear () { > list.first (); ??? Isn't it just identity function? > while (list != null) { > unowned GLib.List<G> n = list; > list.remove_link (n); > } Wouldn't list = null work? Also you need to update size. > } -------------- > public class GListIterator<G> : Object, Traversable<G>, Iterator<G>, ListIterator<G>, BidirIterator<G>, BidirListIterator<G> As a coding convention libgee uses iterators which are private inner classes called Iterator. -------------- General comments: - In most cases you don't need to force recalculate the length of list. - Do we wrap around GLib.List or own GLib.List? In the former case we shouldn't store list in latter we can just calculate at the beginning. - Why do you need GLib.List? While it is easy to implement it have several problems (like for example prepending/appending might change the pointer to list, O(N) size, null being empty list etc.). I am not convinced yet that it would be beneficial to use GLib.List as oppose to moving GXml to Gee.List. If you could explain it I would be grateful.
Review of attachment 225312 [details] [review]: About your last comments, I found that when setup internal GLib.List<G>: > public GLib.List<G> list { get { return _list; } set { _list = value.copy (); } } I need to call copy() witch iterates all over whole list to get the required information, unless I use unowned variable at _list, I need to do that. From your comments: > - Why do you need GLib.List? While it is easy to implement it have several problems (like for example prepending/appending might change the pointer to list, O(N) size, null being empty list > etc.). I am not convinced yet that it would be beneficial to use GLib.List as oppose to moving GXml to Gee.List. If you could explain it I would be grateful. For some reasons GXml creates new Gee abstract container, implements most GLib.List<G> API and then creates a specialized class witch copy all data to internal one. I think this is unnecessary then start to create this new Gee.GList in order to help them to "port" to a new class using just Gee API. But while I'm studding the code at GXml they copy all data, witch I would like to avoid, but Vala force you to do so or use unowned variables, leaving you without control over the data, then I think is much better to use Gee.ArrayList<G> container copy that from libmxl2 (witch uses GLib.List<G>). I would like to avoid to rewrite GXml code to use Gee API and just call internal GLib.List<G> and use its API, make more easy to port, but at the end we will get a lot of unnecessary/duplicated code. May is better to start porting to use just Gee API and drop any GLib.List<G> one, I need to discus these with GXml maintainer.
GXml port to Gee 0.8 is getting at Bug 684910
Could be very useful to know performance and memory benchmarking GList Vs. Gee.LinkedList, or Vs. Gee.ArrayList. A full tests including GLib's containers vs Gee's containers will be very ilustrative to avoid future questions like: "When you must use GLib and when Gee containers"
(In reply to comment #5) > Could be very useful to know performance and memory benchmarking GList Vs. > Gee.LinkedList, or Vs. Gee.ArrayList. > > A full tests including GLib's containers vs Gee's containers will be very > ilustrative to avoid future questions like: > Yes, yes it would. Writing benchmarks is on my TODO list (bug #595826) > "When you must use GLib and when Gee containers" I would say that you shouldn't use GLib.List in Vala unless a) you really need performance and you are worried about cost of CALL and GObject creation or b) you interface with C API as it have an awkward interface (from Vala perspective). In case of actual performance I don't know.