~lattis/muon

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
6 3

[PATCH] Implement version_compare string method

Details
Message ID
<20210713020947.3651-1-fancycade@mycanofbeans.com>
DKIM signature
pass
Download raw message
Patch: +273 -0
---

This does not implement comparing versions with pre-release or build tags.
That is something I would like to get to eventually, but I thought it best
to get feedback on current approach before I continued further on that.

 include/functions/string.h  |   4 +
 src/functions/string.c      | 256 ++++++++++++++++++++++++++++++++++++
 tests/meson.build           |   1 +
 tests/version_compare.meson |  12 ++
 4 files changed, 273 insertions(+)
 create mode 100644 tests/version_compare.meson

diff --git a/include/functions/string.h b/include/functions/string.h
index bb02602..3d80b1b 100644
--- a/include/functions/string.h
+++ b/include/functions/string.h
@@ -8,6 +8,10 @@ enum format_cb_result {
	format_cb_error,
};

struct version {
	long major, minor, patch;
};

typedef enum format_cb_result ((*string_format_cb)(struct workspace *wk, uint32_t node, void *ctx, const char *key, uint32_t *elem));

bool string_format(struct workspace *wk, uint32_t node, uint32_t str, uint32_t *out, void *ctx, string_format_cb cb);
diff --git a/src/functions/string.c b/src/functions/string.c
index cec6fb3..a43d73a 100644
--- a/src/functions/string.c
+++ b/src/functions/string.c
@@ -237,6 +237,261 @@ func_join(struct workspace *wk, uint32_t rcvr, uint32_t args_node, uint32_t *obj
	return obj_array_join(wk, an[0].val, rcvr, obj);
}

static bool
str_to_long(struct workspace *wk, uint32_t rcvr, const char *s, long *output)
{
	char *endptr = NULL;
	*output = strtol(s, &endptr, 10);
	if (s == endptr) {
		interp_error(wk, rcvr, "nondigit in version core: %s", s);
		return false;
	}

	if (*output < 0) {
		interp_error(wk, rcvr, "negative digit in version core: %s", s);
		return false;
	}

	return true;
}

#define MAX_VER_BUF_LEN 64

static bool
str_to_version(struct workspace *wk, uint32_t rcvr, struct version *v, const char *str, size_t start)
{
	char buf[MAX_VER_BUF_LEN];
	uint32_t n = 0;
	size_t j = 0;
	long num = 0;
	for (size_t i = start; str[i]; i++) {

		if (!(j < MAX_VER_BUF_LEN)) {
			interp_error(wk, rcvr, "exceeded maximum buffer length when parsing semantic version");
			return false;
		}

		if (str[i] == '.') {
			buf[j] = '\0';
			if (!str_to_long(wk, rcvr, buf, &num)) {
				return false;
			}

			if (n == 0) {
				v->major = num;
			} else if (n == 1) {
				v->minor = num;
			}

			n++;
			buf[0] = '\0';
			j = 0;
			continue;
		}

		buf[j] = str[i];
		j++;
	}

	if (j < MAX_VER_BUF_LEN) {
		buf[j] = '\0';
	}

	if (!str_to_long(wk, rcvr, buf, &num)) {
		return false;
	}

	v->patch = num;

	return true;
}

static bool
func_version_compare(struct workspace *wk, uint32_t rcvr, uint32_t args_node, uint32_t *obj)
{
	struct args_norm an[] = { { obj_string }, ARG_TYPE_NULL };

	if (!interp_args(wk, args_node, NULL, an, NULL)) {
		return false;
	}

	const char *str = wk_objstr(wk, rcvr);

	struct version v = { 0, 0, 0 };
	if (!str_to_version(wk, rcvr, &v, str, 0)) {
		interp_error(wk, rcvr, "invalid version string");
		return false;
	}

	const char *str_arg = wk_objstr(wk, an[0].val);

	struct version v_arg = { 0, 0, 0 };
	struct obj *res = make_obj(wk, obj, obj_bool);

	if (!strncmp(">=", str_arg, 2)) {
		if (!str_to_version(wk, rcvr, &v_arg, str_arg, 2)) {
			interp_error(wk, rcvr, "invalid version string");
			return false;
		}

		if (v.major > v_arg.major) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.major < v_arg.major) {
			res->dat.boolean = false;
			goto ret;
		}

		if (v.minor > v_arg.minor) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.minor < v_arg.minor) {
			res->dat.boolean = false;
			goto ret;
		}

		if (v.patch > v_arg.patch) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.patch < v_arg.patch) {
			res->dat.boolean = false;
			goto ret;
		}

		res->dat.boolean = true;
	} else if (!strncmp("<=", str_arg, 2)) {
		if (!str_to_version(wk, rcvr, &v_arg, str_arg, 2)) {
			interp_error(wk, rcvr, "invalid version string");
			return false;
		}

		if (v.major < v_arg.major) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.major > v_arg.major) {
			res->dat.boolean = false;
			goto ret;
		}

		if (v.minor < v_arg.minor) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.minor > v_arg.minor) {
			res->dat.boolean = false;
			goto ret;
		}

		if (v.patch < v_arg.patch) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.patch > v_arg.patch) {
			res->dat.boolean = false;
			goto ret;
		}

		res->dat.boolean = true;
	} else if (!strncmp("==", str_arg, 2)) {
		if (!str_to_version(wk, rcvr, &v_arg, str_arg, 2)) {
			interp_error(wk, rcvr, "invalid version string");
			return false;
		}

		res->dat.boolean = v.major == v_arg.major &&
			v.minor == v_arg.minor &&
			v.patch == v_arg.patch;
	} else if (!strncmp("!=", str_arg, 2)) {
		if (!str_to_version(wk, rcvr, &v_arg, str_arg, 2)) {
			interp_error(wk, rcvr, "invalid version string");
			return false;
		}

		if (v.major != v_arg.major) {
			res->dat.boolean = true;
			goto ret;
		}

		if (v.minor != v_arg.minor) {
			res->dat.boolean = true;
			goto ret;
		}

		if (v.patch != v_arg.patch) {
			res->dat.boolean = true;
			goto ret;
		}

		res->dat.boolean = false;
	} else if (!strncmp(">", str_arg, 1)) {
		if (!str_to_version(wk, rcvr, &v_arg, str_arg, 1)) {
			interp_error(wk, rcvr, "invalid version string");
			return false;
		}

		if (v.major > v_arg.major) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.major < v_arg.major) {
			res->dat.boolean = false;
			goto ret;
		}

		if (v.minor > v_arg.minor) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.minor < v_arg.minor) {
			res->dat.boolean = false;
			goto ret;
		}

		if (v.patch > v_arg.patch) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.patch < v_arg.patch) {
			res->dat.boolean = false;
			goto ret;
		}

		res->dat.boolean = false;
	} else if (!strncmp("<", str_arg, 1)) {
		if (!str_to_version(wk, rcvr, &v_arg, str_arg, 1)) {
			interp_error(wk, rcvr, "invalid version string");
			return false;
		}

		if (v.major < v_arg.major) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.major > v_arg.major) {
			res->dat.boolean = false;
			goto ret;
		}

		if (v.minor < v_arg.minor) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.minor > v_arg.minor) {
			res->dat.boolean = false;
			goto ret;
		}

		if (v.patch < v_arg.patch) {
			res->dat.boolean = true;
			goto ret;
		} else if (v.patch > v_arg.patch) {
			res->dat.boolean = false;
			goto ret;
		}

		res->dat.boolean = false;
	} else {
		interp_error(wk, rcvr, "invalid comparison operator");
		return false;
	}

ret:
	return true;
}

const struct func_impl_name impl_tbl_string[] = {
	{ "strip", func_strip },
	{ "to_upper", func_to_upper },
@@ -244,5 +499,6 @@ const struct func_impl_name impl_tbl_string[] = {
	{ "underscorify", func_underscorify },
	{ "split", func_split },
	{ "join", func_join },
	{ "version_compare", func_version_compare },
	{ NULL, NULL },
};
diff --git a/tests/meson.build b/tests/meson.build
index db89f5a..f0df7b6 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -4,6 +4,7 @@ tests = [
	'join_paths.meson',
	'katie.meson',
	'join.meson',
	'version_compare.meson'
]

foreach t : tests
diff --git a/tests/version_compare.meson b/tests/version_compare.meson
new file mode 100644
index 0000000..51bebcd
--- /dev/null
+++ b/tests/version_compare.meson
@@ -0,0 +1,12 @@
version_number = '1.2.8'

assert(version_number.version_compare('>=1.2.8'))
assert(not version_number.version_compare('>1.2.8'))
assert(not version_number.version_compare('<1.2.8'))
assert(version_number.version_compare('<=1.2.8'))
assert(version_number.version_compare('==1.2.8'))
assert(not version_number.version_compare('!=1.2.8'))
assert(not version_number.version_compare('==1.2.9'))

assert(version_number.version_compare('<2.0'))
assert(version_number.version_compare('>0.9'))
-- 
2.32.0
Details
Message ID
<20210713231759.ef2lckiive7eptcg@kuro.my.domain>
In-Reply-To
<20210713020947.3651-1-fancycade@mycanofbeans.com> (view parent)
DKIM signature
pass
Download raw message
> This does not implement comparing versions with pre-release or build tags.
> That is something I would like to get to eventually, but I thought it best
> to get feedback on current approach before I continued further on that.

Thanks!  I think it is OK to leave that stuff for later.

> +struct version {
> +	long major, minor, patch;
> +};
> +

I forgot to mention in the CONTRIBUTING.md, but usually integer types
with specific widths are preferred.  E.g. you should probably use
uint32_t for major, minor, patch.  You already ensure that the parsed
digits are positive.

> +static bool
> +func_version_compare(struct workspace *wk, uint32_t rcvr, uint32_t args_node, uint32_t *obj)
> +{
> +	struct args_norm an[] = { { obj_string }, ARG_TYPE_NULL };
> +
> +	if (!interp_args(wk, args_node, NULL, an, NULL)) {
> +		return false;
> +	}

I think you meant to pass `an` as the 3rd argument (check the signature
for interp_args, you put `an` in the position for optional arguments).

As for the rest of the function, I think the form looks good, but I
wonder if you could remove some code duplication by using a function
pointer for the comparison:

```
static bool
comp_gt(uint32_t a, uint32_t b)
{
	return a > b;
}

static bool
comp_ge(uint32_t a, uint32_t b)
{
	return a >= b;
}

static bool
comp_lt(uint32_t a, uint32_t b)
{
	return a < b;
}

...

typedef bool ((*comparator)(uint32_t a, uint32_t b));

version_compare()
{
	comparator op = comp_eq;

	static struct {
		const char *name;
		comparator op;
	} ops[] = {
		{ ">=", comp_ge, },
		{ ">",  comp_gt, },
		{ "<=", comp_le, },
		...
		NULL
	};

	uint32_t i, op_len;
	for (i = 0; ops[i].name; ++i) {
		op_len = strlen(ops[i].name);

		if (strncmp(str_arg, ops[i].name, op_len)) {
			str_arg = &str_arg[op_len];
			op = ops[i].op;
			break;
		}
	}

...

	if (v.major != v_arg.major) {
		res->dat.boolean = op(v.major, v_arg.major);
		goto ret;
	}

	if (v.minor != v_arg.minor) {
		res->dat.boolean = op(v.minor, v_arg.minor);
		goto ret;
	}
```

I checked meson's implementation[1], and it seems like they support a
few more operators, as well as defaulting to `==` if nothing is matched.
We should probably try to support everything meson does.  (I've also
noticed some strange behaviour like `'1.2.3'.version_compare('!1.2.3!')
== true`)

One other thing, can you make string_version_compare a standalone
function?  We will want to use it for version comparison in things like
`dependency('xxx', version: '>=1.2.3')` which isn't implemented yet.

> diff --git a/tests/meson.build b/tests/meson.build
> index db89f5a..f0df7b6 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -4,6 +4,7 @@ tests = [
>  	'join_paths.meson',
>  	'katie.meson',
>  	'join.meson',
> +	'version_compare.meson'
>  ]

Nice!

[1]: https://github.com/mesonbuild/meson/blob/317f9555a71e70429e8a8ac6c4fe71493c2ea936/mesonbuild/mesonlib/universal.py#L808
Details
Message ID
<8fee60eb42193a5249b38d7c54221b72@mycanofbeans.com>
In-Reply-To
<20210713231759.ef2lckiive7eptcg@kuro.my.domain> (view parent)
DKIM signature
pass
Download raw message
Thanks for the great feedback!

> As for the rest of the function, I think the form looks good, but I
> wonder if you could remove some code duplication by using a function
> pointer for the comparison:
> 


Okay, good idea. This is pushing my limits of C fanciness :D. I should
be able to take your example and expand on it.


> 
> I checked meson's implementation[1], and it seems like they support a
> few more operators, as well as defaulting to `==` if nothing is matched.
> We should probably try to support everything meson does. (I've also
> noticed some strange behaviour like `'1.2.3'.version_compare('!1.2.3!')
> == true`)
> 

Oops, yeah that seems bad. I'll add that as a test case, and add the other 
operators.


> One other thing, can you make string_version_compare a standalone
> function? We will want to use it for version comparison in things like
> `dependency('xxx', version: '>=1.2.3')` which isn't implemented yet.
> 

Is it okay for string_version_compare to live in functions/string.c
and then dependency can import it, or should it be somewhere else?


I'll take another swing at this to address the issues you pointed out
and refactor it a bit.

Harley
Details
Message ID
<20210714114513.va2js2444amgyt7u@kuro.my.domain>
In-Reply-To
<8fee60eb42193a5249b38d7c54221b72@mycanofbeans.com> (view parent)
DKIM signature
pass
Download raw message
> Okay, good idea. This is pushing my limits of C fanciness :D. I should
> be able to take your example and expand on it.

You can do it!

> Oops, yeah that seems bad. I'll add that as a test case, and add the other
> operators.

Just to clarify, you don't have to match the strange
`'1.2.3'.version_compare('!1.2.3!') > == true` behavior, I was just
pointing out something odd.  I have found quite a few of these things
since I started working on muon.

> Is it okay for string_version_compare to live in functions/string.c
> and then dependency can import it, or should it be somewhere else?

It can stay in functions/string.c, we already have string_format() in
the same file, which is used in other places.

Stone
Details
Message ID
<31cc0e1e90df11e57768cce9207a83dc@mycanofbeans.com>
In-Reply-To
<20210714114513.va2js2444amgyt7u@kuro.my.domain> (view parent)
DKIM signature
pass
Download raw message
I have a working version with your suggested changes, but I want to test
it a bit more. I also noticed some oddities with the error messages.

> 
> Just to clarify, you don't have to match the strange
> `'1.2.3'.version_compare('!1.2.3!') > == true` behavior, I was just
> pointing out something odd. I have found quite a few of these things
> since I started working on muon.
> 

Gotcha, well good thing muon is being made! muon will throw an invalid 
version string error on those cases.

This did bring up something about the error messages though. I'm noticing
some strange behavior, and also noticed this on my original patch.

Basically if an error message happens in an assert in the tests I get this output:

```
error version_compare.meson - failed (1)
/home/fc/projects/muon/tests/version_compare.meson:4:51: error: nondigit in version core: !1
  4 | assert(not version_number.version_compare('>1.2.8'))
                                                        ^
/home/fc/projects/muon/tests/version_compare.meson:4:51: error: invalid version string
  4 | assert(not version_number.version_compare('>1.2.8'))
                                                        ^
/home/fc/projects/muon/tests/version_compare.meson:11:1: error: in builtin function assert
 11 | assert(version_number.version_compare('!1.2.8!'))
```

Notice the first line number in the first message isn't correct but the error message is correct.


And then if I try the same thing in a separate project on its own I get this vague error message:

```
Assertion failed: i < da->len (../src/darr.c: darr_get: 141)
Aborted
```

Is this an issue with how I am calling interp_error? Or is this possibly a bug in the error messaging?

Harley
Details
Message ID
<20210715011517.v6ojsddw2w7zcjlz@hostomei>
In-Reply-To
<31cc0e1e90df11e57768cce9207a83dc@mycanofbeans.com> (view parent)
DKIM signature
pass
Download raw message
> Is this an issue with how I am calling interp_error? Or is this possibly a bug in the error messaging?

Ah yes, I re-read your patch, and you aren't calling interp_error
properly.  The second argument to interp_error should be the id of the
AST node where the error occurred.  You passed the id of the method's
receiver object (rcvr), which in the case of your test happens to
actually refer to an AST node (but not the right one). In the case of
your other project, there happens not to be an AST node with the same
object id, so an out-of-bounds assert fires.

Currently inside a function you can either call interp_error with the
`args_node` that you are handed, or, if a specific argument is at fault,
you can do `an[0].node` to get the node of that argument.  This usually
means that support functions also need to get passed an `err_node`, i.e.
the source location of errors.

So, I would modify `str_to_version` to take an additional `err_node`
argument, and use that when throwing errors.

There are a few different concepts all referred to by ID in muon.
Admittedly, this can get pretty confusing.  There are some benefits
though, but I'm thinking of ways to improve the situation!

By the way, I fixed some crashes I found, and also finally added support
for `warning_level`, which helped me find a few more bugs.  I just
pushed.  You might have conflicts with the test file, but I don't think
it will be anything major.

Stone
Details
Message ID
2dbf9728-6c10-46ff-bc9d-1c57abd4c824@mycanofbeans.com
In-Reply-To
<20210715011517.v6ojsddw2w7zcjlz@hostomei> (view parent)
DKIM signature
pass
Download raw message
>
>Currently inside a function you can either call interp_error with the
>`args_node` that you are handed, or, if a specific argument is at fault,
>you can do `an[0].node` to get the node of that argument.  This usually
>means that support functions also need to get passed an `err_node`, i.e.
>the source location of errors.
>

Okay easy enough! I believe I got it working now. The error messages look
much better now:

```
/home/fc/projects/muon/tests/version_compare.meson:11:39: error: nondigit in version core: !1
 11 | assert(version_number.version_compare('!1.2.8!'))
                                            ^
/home/fc/projects/muon/tests/version_compare.meson:11:39: error: invalid version string
 11 | assert(version_number.version_compare('!1.2.8!'))
                                            ^
/home/fc/projects/muon/tests/version_compare.meson:11:23: error: in builtin function string.version_compare
 11 | assert(version_number.version_compare('!1.2.8!'))
                            ^
/home/fc/projects/muon/tests/version_compare.meson:11:1: error: in builtin function assert
 11 | assert(version_number.version_compare('!1.2.8!'))
```

>By the way, I fixed some crashes I found, and also finally added support
>for `warning_level`, which helped me find a few more bugs.  I just
>pushed.  You might have conflicts with the test file, but I don't think
>it will be anything major.
>

Cool!

I'll do a little bit of cleanup, rebase off of master, and send the new patch.

Harley
Reply to thread Export thread (mbox)