Implement naryFun

[rcorre]

naryFun is like unaryFun/binaryFun, but for an arbitrary number of args.
Motivated by #3837

[JakobOvrum]

Is argc really necessary? Looks like parmNames.length.

edit:

I see that it has a path for default parameter names, but this can be added as an overload.

[rcorre]

Unless paramNames isn't given. Then we need argc. I guess I could use an overload instead.

On December 20, 2015 8:50:35 PM EST, JakobOvrum notifications@github.com wrote:

@@ -260,6 +210,83 @@ unittest
static assert(!is(typeof(binaryFun!FuncObj)));
}

+/**
+Transforms a string representing an expression into a n-ary
function.
+The number of arguments is given as $(argc).
+The string must either use symbol names $(D a-z) as the parameters
or
+provide the symbols via the $(D parmNames) arguments.
+If $(D fun) is not a string, $(D naryFun) aliases itself away to $(D
fun).
+*/
+
+template naryFun(size_t argc, alias fun, parmNames...)

Is argc really necessary? Looks like parmNames.length.

---

Reply to this email directly or view it on GitHub:
https://github.com/D-Programming-Language/phobos/pull/3882/files#r48112338

[tsbockman]

While it does seem like 26 parameters ought to be enough for anybody , this arbitrary limitation is not necessary. Fix:

/**
Transforms a string representing an expression into a n-ary function.
$(UL
    $(LI The number of parameters is given by $(D argc). If no explicit
        $(D argc) is given, it is inferred to be $(D parmNames.length).)
    $(LI Explicit names for the function parameters may instead be given as
        $(D parmNames). If more than $(D argc) names are supplied, the
        extras will be ignored.)
    $(LI If no $(D parmNames) are given, the letters ($(D a) through $(D z))
        are used to name up to the first 26 function parameters.)
    $(LI $(D args[index]) can always be used to access any parameter by index,
        regardless of whether it has been given a name.)
)
*/
template naryFun(alias fun, size_t argc, parmNames...)
    if (parmNames.length == 0 || allSatisfy!(isSomeString, typeof(parmNames)))
{
    static if (is(typeof(fun) : string))
    {
        // No need to regenerate the aliases mixin for every ElementTypes permutation.
        private enum aliases = naryFunAliases(argc, parmNames);

        static if (!fun._ctfeMatchNary(parmNames))
        {
            import std.traits, std.typecons, std.typetuple;
            import std.algorithm, std.conv, std.exception, std.math, std.range, std.string;
        }

        auto naryFun(ElementTypes...)(auto ref ElementTypes args)
            if (ElementTypes.length == argc)
        {
            mixin(aliases);
            return mixin(fun);
        }
    }
    else static if (needOpCallAlias!fun)
    {
        // Issue 9906
        alias naryFun = fun.opCall;
    }
    else
    {
        alias naryFun = fun;
    }
}
/// ditto
template naryFun(alias fun, parmNames...)
    if (allSatisfy!(isSomeString, typeof(parmNames)))
{
    alias naryFun = naryFun!(fun, parmNames.length, parmNames);
}

// Generate naryFun parameter aliases mixin string.
private string naryFunAliases(size_t argc, string[] parmNames...) pure
{
    import std.algorithm : min;
    import std.conv : to;

    if(parmNames.length > 0)
    {
        string ret;
        foreach(i, name; parmNames[0 .. min(argc, parmNames.length)])
            ret ~= "alias " ~ name ~ " = args[" ~ i.to!string ~ "];";
        return ret;
    }
    else
    {
        // The default a-z aliases are always the same, so cache them at global scope.
        enum size_t az = ('z' - 'a') + 1;
        enum all = function()
        {
            string ret;
            foreach(i; 0 .. az)
                ret ~= "alias " ~ cast(char)('a' + i) ~ " = args[" ~ i.to!string ~ "];";
            return ret;
        }();
        enum ends = function()
        {
            size_t[az + 1] ret;
            size_t i = 1, j = 0;
            while(i <= az)
            {
                while(all[j++] != ';') { }
                ret[i++] = j;
            }

            return ret;
        }();

        // Uses slices to minimize CT allocations.
        return all[0 .. ends[argc > az? az : argc]];
    }
}

Three other problems solved by the above are:

• Your naryFun isn't actually enforced to be "n-ary". I added the constraint if (ElementTypes.length == argc) to fix this.
• Your code potentially re-evaluates aliases() for every ElementTypes permutation, even though its result depends only upon the outer template arguments. This slows down compilation and leaks memory. Save aliases as an enum outside the inner template to fix this.
• aliases should be private; it is just an implementation detail.

EDIT: Updated to cache the default a through z aliases mixin string at global scope, since it is always the same. I expect this is worth the marginal increase in complexity, since binaryFun is used quite a bit in Phobos.

[tsbockman]

@rcorre I have updated my previous comment; please read the latest version on Github rather than the original email notification.

[rcorre]

@tsbockman I don't think I need to re-add argc as a parameter to the custom-names overload, I could just check if (ElementTypes.length == parmNames.length). Unless you want to support something like naryFun!("x + y + args[2]", 3, "x", "y").

Interesting point about re-evaluating aliases() though -- I had no idea. I'll make it private too.

[rcorre]

Never mind, I see why you did it like that now.

[tsbockman]

@rcorre

Unless you want to support something like naryFun!("x + y + args[2]", 3, "x", "y").

I do want to support that, as a generalized solution to the "cannot generate > 26 parameters" default naming problem.

Will anyone ever use this feature? Who knows!

It costs nothing to expose args[], though, since it's going to be there either way. Perhaps some generic code generator will benefit.

[tsbockman]

Interesting point about re-evaluating aliases() though -- I had no idea.

Some of the generic code I've been working on recently accepts hundreds of different template argument permutations.

Compilation of my exhaustive tests is annoyingly slow - as in C++ slow. The compiler's memory use is kind of disturbing at times, as well. This has got me into the habit of considering the compile-time costs of my CTFE and template use.

It takes some getting used to, but ultimately the only really big differences compared to runtime optimization, are that the CTFE environment is about 100x slower, and leaks like a sieve.

[rcorre]

@tsbockman I basically took your suggestion line-for-line. Counting semicolons in the ends part seemed a bit ugly but I didn't come up with a nicer approach.

This has got me into the habit of considering the compile-time costs of my CTFE and template use.

My (naive) mindset so far has been 'if it doesn't happen at runtime, I can ignore performance' :)

[tsbockman]

@rcorre

Counting semicolons in the ends part seemed a bit ugly but I didn't come up with a nicer approach.

Yes that was a bit of a hack on my part. I thought about cobbling together some things from std.algorithm, but ultimately decided it would complicate things unnecessarily.

My (naive) mindset so far has been 'if it doesn't happen at runtime, I can ignore performance' :)

If the program never finishes compiling, then nothing happens at runtime. Best performance possible! 😉

[rcorre]

I thought about cobbling together some things from std.algorithm, but ultimately decided it would complicate things unnecessarily

The problem with that, as I found out in my earlier attempts, is that this code path is hit by so many other functions. I would have preferred format to that messy string concatenation, but format calls startsWith, which uses a string lambda as a default predicate, and before you know it the compiler is screaming about circular dependencies.

[tsbockman]

The problem with that, as I found out in my earlier attempts, is that this code path is hit by so many other functions. I would have preferred format to that messy string concatenation, but format calls startsWith, which uses a string lambda as a default predicate, and before you know it the compiler is screaming about circular dependencies.

I hadn't thought that far ahead, but it makes sense that circular dependencies would be an issue here.

Counting semicolons in the ends part seemed a bit ugly but I didn't come up with a nicer approach.

Fixed now:

// Generate naryFun parameter aliases mixin string.
string naryFunAliases(size_t argc, string[] parmNames...) pure
{
    import std.array : appender;
    import std.algorithm : min;
    import std.conv : to;

    if(parmNames.length > 0)
    {
        auto ret = appender!string();
        foreach(i, name; parmNames[0 .. min(argc, parmNames.length)])
            ret ~= "alias " ~ name ~ " = args[" ~ i.to!string ~ "];";
        return ret.data;
    }
    else
    {
        // The default a-z aliases are always the same, so cache them at global scope.
        enum size_t az = ('z' - 'a') + 1;
        enum cache = function()
        {
            static struct R
            {
                string all;
                size_t[az + 1] ends;
            }
            R ret;

            auto buff = appender!string();
            foreach(i; 0 .. az)
            {
                buff ~= "alias " ~ cast(char)('a' + i) ~ " = args[" ~ i.to!string ~ "];";
                ret.ends[i + 1] = buff.data.length;
            }

            ret.all = buff.data;
            return ret;
        }();

        // Uses slices to minimize CT allocations.
        return cache.all[0 .. cache.ends[argc > az? az : argc]];
    }
}

[rcorre]

Very clever! I've integrated that now.

[tsbockman]

I somehow dropped the private access modifier here, by accident. Please put it back.

[rcorre]

Re-added private. Also, I replaced argc > az? az : argc with min(az,argc), as you're already importing min for use earlier.

[tsbockman]

To take full advantage of the CT optimizations to naryFun, this should now be:

template unaryFun(alias fun, parmName...)
    if(parmName.length <= 1)
{
    alias unaryFun = naryFun!(fun, 1, parmName);
}
template binaryFun(alias fun, parmNames...)
    if(parmNames.length <= 2)
{
    alias binaryFun = naryFun!(fun, 2, parmNames);
}

Otherwise unaryFun and binaryFun - by far the most common instantiations of naryFun for the foreseeable future - won't benefit from the caching of the default a through z parameter names.

[rcorre]

I do slightly prefer the original signatures. unaryFun(alias fun, parmName = "a") is more clear than unaryFun(alias fun, parmName...) if (parmName.length <= 1).

If we turn naryFunAliases into a template, then multiple instantiations with the same argc won't have to re-evaluate. The problem is that cache would be re-evaluated for each different argc. You can un-nest it from naryFunAliases to avoid this, but I'd rather not create too many top-level private templates just to support this one. I thought maybe static enum would do the trick, but it doesn't look like that works.

I'll just use the changed signatures for now.

[rcorre]

Here's what it looks like as a template with external enumsand nested enums. The pragma is triggered each time for the latter case.

[tsbockman]

I do slightly prefer the original signatures. unaryFun(alias fun, parmName = "a") is more clear than unaryFun(alias fun, parmName...) if (parmName.length <= 1).

It's a little repetitive, but perhaps this is more to your liking?

alias unaryFun(alias fun) = naryFun!(fun, 1);
alias unaryFun(alias fun, string parmName) = naryFun!(fun, 1, parmName);

alias binaryFun(alias fun) = naryFun!(fun, 2);
alias binaryFun(alias fun, string parm1Name) = naryFun!(fun, 2, parm1Name);
alias binaryFun(alias fun, string parm1Name, string parm2Name) = naryFun!(fun, 2, parm1Name, parm2Name);

The point is really just to avoid the parmName = "a" stuff.

[tsbockman]

On lines 278 and 300, I should have written .data.idup, rather than just .data.

The .idup seems to be required to prevent the slicing from triggering a copy - at least at runtime. (Why? I have no idea. You'd think that sticking the value into an enum would make it idup automatically...)

However, this may be moot, as from my testing it looks suspiciously like a copy always occurs if the slicing is performed at compile time. Does DMD not do compile-time string interning (de-duplication)? This seems like low-hanging fruit to cut down on executable size and working set a bit.

[tsbockman]

Anyway, we're splitting hairs at this point; just pick any of the three different fixes we discussed for the unaryFun and binaryFun aliases. (I still prefer my original suggestion, but I don't think it matters much.)

After that, I'd say this is ready to pull.

[rcorre]

Rebased, squashed, and ready to go. I went with the first option you suggested. Thanks for all the insight!

[tsbockman]

Rebased, squashed, and ready to go.

LGTM. 👍 @JakobOvrum ?

[JakobOvrum]

template unaryFun(alias fun, string paramName)

[tsbockman]

@JakobOvrum That would change the semantics, as parmNames.length <= 2, not parmNames == 2.

An alternative we considered earlier would be this:

alias unaryFun(alias fun) = naryFun!(fun, 1);
alias unaryFun(alias fun, string parmName) = naryFun!(fun, 1, parmName);

alias binaryFun(alias fun) = naryFun!(fun, 2);
alias binaryFun(alias fun, string parmName) = naryFun!(fun, 2, parm1Name);
alias binaryFun(alias fun, string parmName0, string parmName1) = naryFun!(fun, 2, parmName0, parmName1);

Is that acceptable?

[JakobOvrum]

How about the old signatures using default arguments?

[tsbockman]

@JakobOvrum Specifying the default arguments in that way defeats the caching of the naryFunAliases() string mixin on line 291. This would make compilation slower and more memory hungry for programs which instantiate binaryFun or unaryFun many times.

Given this is exactly the sort of template which is likely to be deeply nested inside of others, I'd like it to minimize its compile-time costs.

[andralex]

Better use the backticks in new code instead of $(D ...)

[rcorre]

would it be $(LREF unaryFun) or just unaryFun?

[rcorre]

@JakobOvrum it is possible to keep the original signature and leverage the cached strings by making naryFunAliases a template, but it involves un-nesting some private enums so they aren't re-evaluated for each template instantiation.

[rcorre]

How about the commit I just added? Making naryFunAliases a template simplifies the code and only instantiates once for each argc. Assuming most people won't want argc > 3 or so, it probably won't be too much work.

[wilzbach]

The ё used in some of the current test cases is a two-byte code point under UTF-8. Is that sufficient?

Oh I remembered it wrongly to be in the first byte. Yep that's sufficient for me :)

[tsbockman]

Ping @JakobOvrum @9il @andralex .

Can we move forward with this? It's blocking another PR by @rcorre .

[rcorre]

Any updates here? Waiting on this to improve #3837.

[JakobOvrum]

I've hinted at this before, but I think the argc parameter is overengineering. Does stringLambda!(foo, 26) offer any advantage over stringLambda!(foo, aliasSeqOf!(std.ascii.lowercase[]))? stringLambda!(foo, 4, "x", "y") over stringLambda!(foo, "x", "y", aliasSeqOf!(lowercase[0 .. 2]))? The rules for argc are quite complex and it adds a certain amount of complexity to the implementation as well.

[rcorre]

@JakobOvrum I'd say no. It was imitating the use of default names for unaryFun and binaryFun, but I don't see a problem with having to explicitly specify names if you're parsing a string lambda with > 2 params. Actually, that might be preferable. I'll see how it looks without it.

[rcorre]

@JakobOvrum : see those two commits for what it looks like without argc. Reasonably simpler implementation, without losing much value (IMHO). Actually I think eliminating the automatic usage of a..z makes the use clearer.

This does miss the case where someone wants a 50-param function and doesn't want to pass in 50 param names (they just want to use args[n]). But I think that's an edge case and its reasonable to ask them to generate parameters.

Your aliasSeqOf!(lowercase[]) doesn't quite work as we expect string param names. We could either complicate the implementation or just suggest using aliasSeqOf!(lowercase[].map!(x => x.to!string)). I went with the latter.

[9il]

I've hinted at this before, but I think the argc parameter is overengineering.

This PR is complete overengineering and I would like to close it with related PRs.

[rcorre]

I'm ok with that. I don't feel particularly strongly about this and have other PRs I'd rather focus on. Any other opinions on closing this and possibly #3837?

[JakobOvrum]

This PR is complete overengineering and I would like to close it with related PRs.

@9il, please, this is not a productive comment. The motivations here have been clearly stated. What are the authors supposed to do when presented with a comment like this?

Please don't close pull requests (or threaten to) simply because you don't like them. If you think something is a bad idea, please elaborate on why - there's a chance your comment will guide the work in a good direction.

---

@rcorre, I think the current proposal makes a lot of sense, but I hope to hear from @9il about his considerations.

[9il]

@9il, please, this is not a productive comment. The motivations here have been clearly stated. What are the authors supposed to do when presented with a comment like this?

I contributed to this PR too, because I was afraid that this PR can be merged into Phobos and I wanted to reduce issues of this PR. So, I tried to improve it. Later, when I was included to Phobos core team I decide to do not spend time on it. This PR wanted to be improvement of my import workaround for string lambdas. I will not describe why I don't like it to be merged because this is about style/time/architecture and not about algorithms and clear reasons. This is just my opinion.

[tsbockman]

This PR is complete overengineering and I would like to close it with related PRs.
...
This PR wanted to be improvement of my import workaround for string lambdas. I will not describe why I don't like it...

While there are probably other legitimate criticisms that could be made of my work on strLambdaNeedsImports(), I really don't understand how it, specifically, can be dismissed as "overengineered".

That's a term usually used to attack excessively complex solutions, but strLambdaNeedsImports() is actually simpler than the _ctfeMatch system it replaces:

• It's about 30 or 40 lines shorter,
• It has two fewer external dependencies,
• It has four fewer module-level symbols,
• It eliminates a duplicated unit test block, and
• It is (in my opinion) much easier to understand.

[rcorre]

That's true, I think there was a fair bit of duplicate code between unaryFun and binaryFun that was consolidated.

@tsbockman could you double-check me on that last commit? I think the buffer length guess was off by 1.

[tsbockman]

@rcorre

could you double-check me on that last commit? I think the buffer length guess was off by 1.

That commit is not helping anything. The buffer was already large enough for typical usage, and in the rare case that someone is using long variable names, this bit will grow it as needed:

while(ret_buff.length < (ret.length + line + name.length + i.length))
    ret_buff.length *= 2;

What problem are you trying to solve?

[tsbockman]

I think the buffer length guess was off by 1.

The initial size of that buffer has no significance, other than as a performance optimization. The code should still work as long as the initial size is greater than zero and not too ridiculously large.

[tsbockman]

@rcorre Looking at this again (it's been a while since I wrote this stuff)...

You should undo that change; it messes up the (formerly) perfect length calculation in the while loop I quoted above. If for some reason you really need to change the initial buffer size, the correct way to do so is to modify this part:

auto ret_buff = new char[paramNames.length * (line + 8)];

In particular, the + 8 is the adjustable parameter.

[rcorre]

I assumed it was guessing the length of a single variable name as an optimization, and it looked wrong based on the string it was generating. I didn't read carefully enough. Undoing it now.

[wilzbach]

That's true, I think there was a fair bit of duplicate code between unaryFun and binaryFun that was consolidated.

I do agree with @tsbockman and @rcorre that this is pretty useful.

[andralex]

unaryFun and binaryFun were added at a time when there was no short lambda syntax. Lately that has rendered them all but unnecessary - short lambdas are better integrated with the language and cleaner.

So this adds a considerable amount of sophistication to an approach that is already being obsoleted. There's also a sort of decline in usefulness - string lambdas are best when the body is very short, but the more arguments functions have the more bulky body they also tend to have.

I might be missing something, e.g. uses of the imports analysis or other ancillary functions. But if the sole purpose of this is to generalize unaryFun and binaryFun to multiple arguments, it gets my veto.

I'll close this; feel free to reopen or redo if it seems I'm mistaken.