Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

elektraKeyCmpOrder with order = 0 #3274

Open
bauhaus93 opened this issue Nov 24, 2019 · 3 comments

Comments

@bauhaus93
Copy link

@bauhaus93 bauhaus93 commented Nov 24, 2019

Does a order metakey with value 0 have any special meaning (I couldn't find any information on it in METADATA.ini)?

Otherwise I think the elektraKeyCmpOrder function does not work as intended. While working on the toml plugin, I got an unexpected ordering of my Key array after sorting it with the help of elektraKeyCmpOrder.

Expected Result

I expected a Key with order metakey 0 to be sorted before a key with order 1.

Actual Result

The Key with order = 0 wasn't sorted as first element, but between the keys with order 101 and 102.

Steps to Reproduce the Problem

When I looked into the implementation of elektraKeyCmpOrder I saw that when I compare 0 with any other positive number, none of the if branches will get taken and the return 0 is taken, after the cannot happen anyway comment.
Relevant elektraKeyCmpOrder code:

if (aorder > 0 && border > 0) return aorder - border;
if (aorder < 0 && border < 0) return 0;
if (aorder < 0 && border >= 0) return -1;
if (aorder >= 0 && border < 0) return 1;
/* cannot happen anyway */
return 0;

For example, let aorder be 0 and border be 1, then no branch is taken since

(0 > 0 && 1 > 0) -> false
(0 < 0 && 1 < 0) -> false
(0 < 0 && 1 >= 0) -> false
(0 >= 0 && 1 < 0) -> false

or if aorder = 1 and border = 0

(1 > 0 && 0 > 0) -> false
(1 < 0 && 0 < 0) -> false
(1 < 0 && 0 >= 0) -> false
(1 >= 0 && 0 < 0) -> false

For both cases the function returns 0, meaning that the keys are equal in ordering.

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 24, 2019

Thank you for reporting this bug!

Does a order metakey with value 0 have any special meaning

No.

after the cannot happen anyway comment.

Sounds like a bug, an assert would have been helpful there.

Feel free to change the implementation of elektraKeyCmpOrder as you want.

One idea was to also allow suborders like 0.1.2. This might be useful for sections (the first number counts the section, the second the key and so on).

In any case, please improve the docu in doc/METADATA.ini. At the moment the docu does not contain any example, nor which range of numbers is allowed (which seems to have made you very unsure if this is a bug).

@kodebach

This comment has been minimized.

Copy link
Contributor

@kodebach kodebach commented Nov 25, 2019

One idea was to also allow suborders like 0.1.2.

If we prefix the integer(s) with _ (like we do in array element keys), a simple strcmp should deliver the expected result. Then there would be no need for atoi (which is another bug waiting to happen):

int elektraKeyCmpOrder (const Key * ka, const Key * kb)
{
        // keyGetMeta is NULL, if key is NULL
	const Key * kam = keyGetMeta (ka, "order");
	const Key * kbm = keyGetMeta (kb, "order");

	if (ka == kb) return 0; // same pointer (including both NULL)
	if (ka == NULL) return -1;
	if (kb == NULL) return 1;

	return strcmp (keyString (kam), keyString(kbm));
}

If the special case for negative numbers has to be preserved, this won't work of course.

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 25, 2019

Yes, having a simple strcmp with with /#1/#_10 arrays would also be possible. But this would require to rewrite all plugins....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.