Suggest adding three trivial overloads for a more uniform use of expr() about matchit.cpp

[BowenFu]

Hi @hugoetchegoyen, thanks for bringing this up and that is a very good topic.

"expr" was introduced one year ago and now we have a chance to rethink about it.
Maybe there is a chance to remove all its usage and provide overloaded versions for
nullary callable, Id, and others for "when" and "operator=".

Then the sample would be like

#include "matchit.h"

template<typename T1, typename T2>
constexpr auto eval(std::tuple<char, T1, T2> const& exp)
{
    using namespace matchit;
    Id<T1> i;
    Id<T2> j;
    return match(exp)
    (
        pattern | ds('+', i, j) = i + j,
        pattern | ds('-', i, j) = i - j,
        pattern | ds('*', i, j) | when(i) = i, // previously not allowed
        pattern | ds('/', i, j) = 12345,        // previously not allowed
        pattern | _ = []
        {
            assert(false);
            return -1;
        }
    );
}

The only concern is about others. Would that be too risky if users provide a unary instead of nullary mistakenly and we silent the error and return the unary function as the result?

A compromise may be to keep expr for others only and deprecate its usage for Id.

What do you think?

Thanks,
Bowen

[hugoetchegoyen]

Hi Bowen, Thanks for your reply. I like the idea of eliminating the need to use expr() entirely by overloading operator= and when() for nullary callables, Ids and others. I realize that if the user mistakenly uses a unary instead of a nullary, the unary will fall in the category of others and will be used as a plain value, not called. I don't think this deserves a special protection. I would not deprecate expr() but keep it around as an option, this way you don´t break backward compatibility. In some cases you might also like to use expr() for cosmetic reasons. For example, consider a handler that returns a nullary lambda: pattern | _ = []{ return []{ return 123; }; } In my opinion it looks nicer with expr(): pattern | _ = expr([]{ return 123; }) Best regards, Hugo El mié, 9 nov 2022 a las 10:46, Bowen Fu ***@***.***>) escribió:

…

Hi @hugoetchegoyen <https://github.com/hugoetchegoyen>, thanks for bringing this up and that is a very good topic. "expr" was introduced one year ago and now we have a chance to rethink about it. Maybe there is a chance to remove all its usage and provide overloaded versions for nullary callable, Id, and others for "when" and "operator=". Then the sample would be like #include "matchit.h" template<typename T1, typename T2>constexpr auto eval(std::tuple<char, T1, T2> const& exp) { using namespace matchit; Id<T1> i; Id<T2> j; return match(exp) ( pattern | ds('+', i, j) = i + j, pattern | ds('-', i, j) = i - j, pattern | ds('*', i, j) | when(i) = i, // previously not allowed pattern | ds('/', i, j) = 12345, // previously not allowed pattern | _ = [] { assert(false); return -1; } ); } The only concern is about others. Would that be too risky if users provide a unary instead of nullary mistakenly and we silent the error and return the unary function as the result? A compromise may be to keep expr for others only and deprecate its usage for Id. What do you think? Thanks, Bowen — Reply to this email directly, view it on GitHub <#94 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ2LRV6SFAMDHQUWNDL5XW3WHOTKZANCNFSM6AAAAAAR22EK5A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[hugoetchegoyen]

PS:

With my three trivial overloads the situation is like your second alternative: expr() is compulsory for others only. But it is not deprecated, on the contrary, it is generalized to accept everything.

[BowenFu]

@hugoetchegoyen good pointer about "this doesn't deserve a special protection". And by deprecate I mean marked it with "[[deprecated]]", so to discourage its usage and remove in a later major version release.

If you are interested, will you create a pull request for it?

Thanks,
Bowen

[hugoetchegoyen]

Bowen,

I already made a quick try to overload when() and operator= for others too, but failed, I will make a more serious attempt as soon as I find some free time and I will let you know. Then if you are satisfied with the result I can make a pull request, but first I have to find out how since my git skills are very limited.

Regards,
Hugo

[hugoetchegoyen]

Hi Bowen,

I think I suceeded. The changes are very small. You can use 'others' as when predicates or handlers now. I attach a file with the changes and the sample program. Function expr() is still available, I used it (unnecessarily) in expr(i+j). If you would like to remove it, I guess it can be moved to namespace impl.

Regards,
Hugo

matchit2.txt

[BowenFu]

@hugoetchegoyen looks good to me. And you can also define a helper toNullary function inside impl namespace and than you will only need one version of when / operator=.

Are you planning to create a pull request yourself or you want me to create one for you?
Please let me know if you meet any issues when creating a PR.
To create a PR, you need to fork the repo and commit the changes to your fork repo firstly and then create a PR from your fork repo to this repo.
You can find more info here. Hope this helps.
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork

Thanks,
Bowen

[hugoetchegoyen]

Hi Bowen,

Here is a version with the toNullary helper function, please advise if you prefer this one.

Still, I could not merge the two versions of when/operator=, The general case takes a const reference and that cannot be changed because otherwise it does not bind to temporaries. The Id version takes a non-const reference and that cannot be changed either because get() and operator* of Ids are not const. So I had to keep them separate. The only way would be to have a const version of operator* or get() for Ids.

Best regards,
Hugo

matchit3.txt

[BowenFu]

Hi @hugoetchegoyen , you can try with universal reference:

        template <typename T>
        constexpr auto toNullary(T&& v)                // added
        {
            if constexpr ( std::is_invocable_v<std::decay_t<T>> )
            {
                return v;
            }
            else
            {
                return expr(v);
            }
        }

[hugoetchegoyen]

Hi Bowen,

Great! The universal references solved it. If you think it's OK now I'll create a pull request.

Regards,
Hugo

matchit4.txt

[hugoetchegoyen]

PS sorry for the mistake, I did use std::decay_t in toNullary. Sent you an outdated file.

[hugoetchegoyen]

This is the updated file.

matchit4.txt

[hugoetchegoyen]

I already forked your repository, commited my changes, built the targets and ran unittest without errors, so it seems the changes don't break existing code. I'll create a pull request as soon as you agree.

[BowenFu]

LGTM. Feel free to create a pull request. We can continue our discussions there (if any).

And can you also add some unit tests to cover the new code paths?

And actually you can implement main functionality in toNullary and delegate expr to toNullary. Then "expr(Nullary const &v)" is no longer needed since you can use impl::toNullary instead.

[BowenFu]

@hugoetchegoyen I'm going to close this issue if there is no objection.
And I've added your name to the contributor section of the README. Thanks for your contribution.

[hugoetchegoyen]

Thank you very much Bowen.
Best regards,
Hugo