Add sort option to `is` #13

Merged

Dr-Carlos merged 2 commits from Dr-Carlos/fxos:sort-is into master

2022-12-23 21:30:27 +01:00

Dr-Carlos commented

2022-12-22 23:23:42 +01:00

Collaborator

Hello!

This pull request simply adds a sort option to is, sorting by type (syscall then address) and then by value.

I would probably have just pushed this to upstream but wanted to check if the operator< implementation I used to allow this sorting was the right way of doing this.

Hello! This pull request simply adds a sort option to `is`, sorting by type (syscall then address) and then by value. I would probably have just pushed this to upstream but wanted to check if the `operator<` implementation I used to allow this sorting was the right way of doing this.

Dr-Carlos added 1 commit 2022-12-22 23:23:46 +01:00

b494a30404 _is: add sort option

Lephenixnoir commented

2022-12-22 23:28:02 +01:00

Owner

The code for the operator doesn't seem quite right to me? In order to sort by syscall number then by address, you need a lexicographic order, which something like

bool operator<(const FxOS::Symbol &right) const
{
    return (type < right.type) || (type == right.type && value < right.value);
}

instead of

bool operator<(const FxOS::Symbol &right) const
{
    return (type < right.type) || (value < right.value);
}

which is just a product order. Since the product order is not total, you can't really sort with it.

Other than this, the implementation feels right to me. I'd just add that if there are several interesting orders (eg. by address) and no canonical one then I'd write a set of compare_this_way() methods.

The code for the operator doesn't seem quite right to me? In order to sort by syscall number then by address, you need a lexicographic order, which something like ```cpp bool operator<(const FxOS::Symbol &right) const { return (type < right.type) || (type == right.type && value < right.value); } ``` instead of ```cpp bool operator<(const FxOS::Symbol &right) const { return (type < right.type) || (value < right.value); } ``` which is just a product order. Since the product order is not total, you can't really sort with it. Other than this, the implementation feels right to me. I'd just add that if there are several interesting orders (eg. by address) and no canonical one then I'd write a set of `compare_this_way()` methods.

Dr-Carlos added 1 commit 2022-12-22 23:51:32 +01:00

efaad5b980 add extra symbol comparison methods

Dr-Carlos commented

2022-12-22 23:52:47 +01:00

Author

Collaborator

The code for the operator doesn't seem quite right to me? In order to sort by syscall number then by address, you need a lexicographic order, which something like
bool operator<(const FxOS::Symbol &right) const
{
    return (type < right.type) || (type == right.type && value < right.value);
}
instead of
bool operator<(const FxOS::Symbol &right) const
{
    return (type < right.type) || (value < right.value);
}
which is just a product order. Since the product order is not total, you can't really sort with it.

Yep, makes sense. I have fixed this.

Other than this, the implementation feels right to me. I'd just add that if there are several interesting orders (eg. by address) and no canonical one then I'd write a set of compare_this_way() methods.

I've added the other methods as they should be (very slight) optimisations if anyone uses them, and also allow == comparisons.

> The code for the operator doesn't seem quite right to me? In order to sort by syscall number then by address, you need a lexicographic order, which something like > > ```cpp > bool operator<(const FxOS::Symbol &right) const > { > return (type < right.type) || (type == right.type && value < right.value); > } > ``` > > instead of > > ```cpp > bool operator<(const FxOS::Symbol &right) const > { > return (type < right.type) || (value < right.value); > } > ``` > > which is just a product order. Since the product order is not total, you can't really sort with it. Yep, makes sense. I have fixed this. > Other than this, the implementation feels right to me. I'd just add that if there are several interesting orders (eg. by address) and no canonical one then I'd write a set of `compare_this_way()` methods. I've added the other methods as they should be (very slight) optimisations if anyone uses them, and also allow `==` comparisons.

Lephenixnoir commented

2022-12-23 15:40:16 +01:00

Owner

I've added the other methods as they should be (very slight) optimisations if anyone uses them, and also allow == comparisons.

Note that the standard library basically only uses == and <, and I think (but I'm not sure here) the others might be derivable. Anyways, LGTM; merge at will :)

> I've added the other methods as they should be (very slight) optimisations if anyone uses them, and also allow == comparisons. Note that the standard library basically only uses `==` and `<`, and I think (but I'm not sure here) the others might be derivable. Anyways, LGTM; merge at will :)

Lephenixnoir approved these changes 2022-12-23 15:40:59 +01:00

Dr-Carlos commented

2022-12-23 21:30:11 +01:00

Author

Collaborator

Note that the standard library basically only uses == and <, and I think (but I'm not sure here) the others might be derivable. Anyways, LGTM; merge at will :)

Yes, the others are totally derivable, but, this would take marginally more processing time, so I have added them all for completeness. Merged.

> Note that the standard library basically only uses `==` and `<`, and I think (but I'm not sure here) the others might be derivable. Anyways, LGTM; merge at will :) Yes, the others are totally derivable, but, this would take marginally more processing time, so I have added them all for completeness. Merged.

👍 1

Dr-Carlos merged commit 5e20cbe805 into master

2022-12-23 21:30:27 +01:00

Dr-Carlos deleted branch sort-is

2022-12-23 21:30:28 +01:00

Dr-Carlos referenced this issue from a commit

2022-12-23 21:30:28 +01:00

Merge pull request 'Add sort option to `is`' (#13) from Dr-Carlos/fxos:sort-is into master

No reviewers