make ApplyLeft/ApplyRight work with deep template

[mogud]

The added codes in unittest not work before.

[wilzbach]

right?

[dnadlinger]

Does the alias ApplyLeft = Id!(Template!(args, right)); trick work here (where Id is a template that does the alias-ing/enum-ing)? (which would leave you with only one branch)

[mogud]

@wilzbach I edited it on web. It's a mistake caused by ctrl + c/v. :(

[dnadlinger]

To elaborate on my in-line comment, given this (which certainly exists somehwere in Phobos)

alias Id(T) = T;
alias Id(alias T) = T;

the implementation could simply be

template ApplyLeft(alias Template, args...)
{
    alias ApplyLeft(right...) = Id!(Template!(args, right));
}

template ApplyRight(alias Template, args...)
{
    alias ApplyRight(left...) = Id!(Template!(left, args));
}

I'm not sure why treating the zero-argument case differently would be required. Maybe that's an attempt at (premature?) optimisation; the original PR discussion should hopefully have more information.

[dnadlinger]

Thanks for adding the unit test; as you discovered, the current implementation is not nearly covered as well as it should be. However, I don't think it really makes sense to show this particular case in the documentation, as it looks pretty confusing at first, and users would just expect it to work transparently anyway. Could you move it to a non-DDoc unittest block?

[mogud]

Agreed. Id -> Alias in meta.d? I tried the new implementation used Alias and all tests passed.

[dnadlinger]

If you want to go this route instead of the simplified version (which is fair enough for a quick fix), could you please make sure that the unit test actually covers both the cases? It seems like the typo you had previously made in ApplyLeft slipped through the cracks.

[dnadlinger]

The Id template I was looking for is called Alias in Phobos, albeit with slightly different (less elegant? ;P) implementation. I'd suggest using that instead of duplicating its implementation here.

[mogud]

I mentioned Alias in line note previously :). For a more elegant way to implement this, I do not know if we may also reimplement Alias to Id's you write out. As far as I know, alias can also used where an enum is needed. So why an enum case is required? I'm a complete newbie in d for only a few weeks experience.

[dnadlinger]

@mogucpp: Sorry about that, the GitHub UI makes such comments easy to miss. The basic issue here is that (simplifying a bit) alias can only be used on types and symbols, not values. For example, alias a = 3; would not compile. However, enum a = 3; does, so one way to write these generic templates is to explicitly switch to using enum when required. However, littering all the code with two branches is unsatisfactory. This is where Alias helps – you can use it to transparently "convert" values into symbols. This way, only one case is required.

[mogud]

Sorry for my poor English misleading. I mean the current implement of Alias use static if for enum and alias cases. And in my opinion, it's not necessary.

[dnadlinger]

Please leave those as a paragraph break.

[dnadlinger]

In this case, you are right. I don't know of a case where that would be required either. That being said, the fact that you can always alias alias parameters is somewhat of a little-known implementation detail, so whoever wrote and reviewed Alias might not have been aware of it. (Or my memory is outdated and there are actually cases now where the simple version doesn't work.)

[mogud]

This PR's commits looks awful. So weak I am. :(

[dnadlinger]

Ah, we've all started out at some point. What would be embarrassing is if I didn't remember any of the old tricks – after all, my name is on that module, too. ;)

Apart from that, we generally ask people to squash their commits into one (or atomic pieces) before merging a PR anyway.

[mogud]

All commits squashed into one now. Thanks for your patience. :)

[DmitryOlshansky]

This also fixes issue https://issues.dlang.org/show_bug.cgi?id=16070

See also #4352

UPDATE: so this pull seems to be a superset of #4352 - @klickverbot which one to merge? ;)

[dnadlinger]

@DmitryOlshansky: Merge the other one of you are sure that it also works for all value cases, this one to play it safe. ;)

[dnadlinger]

@mogucpp @aG0aep6G: There has been a bit of unfortunate duplication of work going on between this and #4352 – could you guys collaborate on merging the test cases together into one PR?

[mogud]

Covered all cases now, i think.

[DmitryOlshansky]

Just add "fixes Issue 16070" to commit message and it should be good to go.

[aG0aep6G]

Just add "fixes Issue 16070" to commit message and it should be good to go.

I think @mogucpp forgot to leave a note here. The commit message now contains "fix Issue 16070".

[DmitryOlshansky]

Auto-merge toggled on