Yes, we should improve the built-in arrays.

Created attachment 160268 [details] [review]
Patch that implements Array.sort() and Array.shuffle()

The attached patch implements sort() and shuffle() operations for build in arrays. See an example below:

-----

static void main (string[] args)
{
	var cities = new string[] { "Helsinki", "Stockholm", "Oslo", "London", "Paris", "Berlin", "Amsterdam", "Madrid" };

	cities.sort ();
	foreach (string city in cities) {
		print ("%s\t", city);
	}
	print ("\n\n---\n\n");

	cities.shuffle ();
	foreach (string city in cities) {
		print ("%s\t", city);
	}
	print ("\n");
}

Forgot to mention that the patch supports object types as well so long as the object has 'int compare (...)' method. See the example below:

-----

public class Currency : Object {
	public string symbol;
	public double usd_rate;
	public Currency (string sym, double usd_rate)
	{
		this.symbol = sym;
		this.usd_rate = usd_rate;
	}
}

public class Price : Object {
	public double price;
	public Currency currency;
	public Price (double price, Currency currency)
	{
		this.price = price;
		this.currency = currency;
	}
	public int compare (Price p)
	{
		double v1 = price / currency.usd_rate;
		double v2 = p.price / p.currency.usd_rate;
		if (v1 == v2) {
			return 0;
		} else if (v1 < v2) {
			return -1;
		} else {
			return 1;
		}
	}
	public string to_string ()
	{
		return "%g %s".printf (price, currency.symbol);
	}
}


static void main (string[] args)
{
	var usd = new Currency ("USD", 1.0000);
	var eur = new Currency ("EUR", 0.7690);
	var jpy = new Currency ("JPY", 94.3650);

	var prices = new Price[] { 
		new Price (26.39, usd), 
		new Price (16.50, usd), 
		new Price (12.99, eur), 
		new Price (1919.00, jpy)
	};

	print ("--in ascending order--\n\n");
	prices.sort ();
	foreach (Price price in prices) {
		print ("%s\t", price.to_string ());
	}
	print ("\n\n--in random order--\n\n");

	prices.shuffle ();
	foreach (Price price in prices) {
		print ("%s\t", price.to_string ());
	}
	print ("\n");
}

Created attachment 160735 [details] [review]
Patch that implements Array.sort(), Array.reverse() and Array.shuffle()

This patch adds the following:

* Support for both ascending and descending sorting order. Ascending sort is arr.sort() or arr.sort(false) and descending sort is arr.sort(true).
* Array.reverse() method reverses the order of elements in the array.
* Some clean up in valaccodemethodcallmodule.vala

Thanks Jukka! for working on this, especially the capability to sort objects by its compare method is very useful, at least for me because provides similar functionality as:
http://es.php.net/manual/en/function.usort.php
http://es.php.net/manual/en/function.uasort.php

which are used to sort associative arrays by a specific sort function, I use them in personal php programs that I could now translate to vala just replacing my assoiciate arrays with array of objects.

Looking forward to see your patch merged :-)

This is just great! Great work Jukka. I'm also this lands quickly in upstream.

I'm chiming in late, but personally I feel this approach is not parallel to the
rest of Vala (and Gee).  A CompareFunc delegate is normally used to determine
ordering.  It feels to me that would be appropriate here as well.

The proposed solution limits sorting to the elements' natural order, whereas I
could easily see sorting objects and primitives on multiple criteria.  It may
be that Array.sort() could do something like Gee.Functions, where if no
CompareFunc is provided (defaults to null) it attempts to find a comparator
that is correct for the elements.  In other words:

void sort(CompareFunc? compare_func = null)

This would also do away with the reverse() method, as that's a function of the comparator.

I do like this feature, though, and would like to see it implemented.

Ok, I will create a new patch which has an optional CompareFunc as an argument. But I still think that reverse() is usuful as a separate array operation. I just looked a few other platforms, and Array.reverse() is in all of them.

Now that I think about it, reverse() does make sense merely as an operation to reverse the order of the elements, whether they've been sorted or not.

Review of attachment 160735 [details] [review]:

Very interesting indeed, I've looked at the patch and noticed some inconsistencies, it needs a careful review, but it looks good to me. Keep up the good work :-)

btw, Splinter doesn't seem to like your patch, the folloing comments are on valaccodearraymodule.vala

::: vala-0.8.1/vala/valaarraytype.vala
@@ +762,3 @@
+		string[] simple_types;
+		if (context.profile == Profile.GOBJECT) {
+			simple_types = new string[] { "gunichar", "char", "guchar", "double", "float", "long", "ulong", "int", "uint", "short", "ushort", "gsize", "gint8", "guint8", "gint16", "guint16", "gint32", "guint32", "gint64", "guint64" };

Please use either Vala names or C names (whichever make sense, I didn't read carefully), you're using double and ulong but gsize and gunichar

@@ +799,3 @@
+				if (comptype in simple_types) {
+					if (i == 1) {
+						fun.block.add_statement (new CCodeReturnStatement (new CCodeConstant ("(pa == NULL && pb == NULL) ? 0 : (pa == NULL) ? 1 : (pb == NULL) ? -1 : (*pa < *pb) ? 1 : ((*pa == *pb) ? 0: -1)")));

having "(pa == NULL && pb == NULL) ? 0 : (pa == NULL) ? 1 : (pb == NULL) ? -1 : (*pa < *pb) ? 1 : ((*pa == *pb) ? 0: -1)" as a constant is a bit too much. And, although not very important, you could make it more readable (see for example append_vala_strcmp0)

Created attachment 160890 [details] [review]
Patch that adds Array.sort(), Array.reverse() and Array.shuffle()

This patch adds the following:

* Uses CompareFunc argument for array.sort() instead of bool for reversed order.
* Uses g_strcmp0 for strings in gobject profile.
* Added a comment on the type names in the array (I don't pick the names, they are C names of Vala types).

Created attachment 160892 [details] [review]
Patch that adds Array.sort(), Array.reverse() and Array.shuffle()

Sorry, my mistake. I actually picked some of the names. This fixes the issue.

Btw, it seems that Vala uses double and float instead of gdouble and gfloat. That confused me.

Ping... Any interest in getting this integrated?

This is absolutely needed... Vala should allow to set such functions. Please, integrate it!

Up

(In reply to comment #12)
> Created an attachment (id=160892) [details] [review]
> Patch that adds Array.sort(), Array.reverse() and Array.shuffle()

I'd love to see a review for this patch.

Hi, thanks for the patch, however:
- It's old and needs to be ported to the new codegen api.
- Please avoid the new compare magic method concept, always require a CompareFunc.
- Use CompareDataFunc instead of CompareFunc if possible.
- Why memcpy? A simple assignment ought to be enough. Also use the stack instead of allocating the temp on the heap.

Created attachment 336104 [details] [review]
Updated patch

Targeted to 1.2 Release. Hope any one can review or change milestone if needed.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/7.