Add array target template

[denis-sh]

Yes, again.

But this time with supplemental changes showing that ArrayTarget is just as frequently used as ElementEncodingType and makes code cleaner.

This pull is easier to review by commits.

[jmdavis]

1. How is ArrayTarget different from ElementEncodingType. It looks to me like it's doing the same thing.
2. How on earth is the term "target" applicable here? Arrays don't have targets. They're not shooting or aiming at anything.

[denis-sh]

How is ArrayTarget different from ElementEncodingType. It looks to me like it's doing the same thing.

The main difference is name. When we see ArrayTarget we know it's an array (and the name is shorter). And it checks that its argument is an array (see this diff).
And one doesn't have to import std.range when he writes some templates with arrays.

How on earth is the term "target" applicable here? Arrays don't have targets. They're not shooting or aiming at anything.

Something like this I suppose: arrays are pointers in C => array target is like a pointer target.
The title is from Issue 5061 - std.traits.arrayTarget.
And of course it may be changed.

[jmdavis]

I grant you that ElementEncodingType is an unfortunately long name, but I do not think that either its overly long name or a desire to avoid importing std.range is enough reason to create an entirely new template to do essentially the same thing. Maybe other Phobos devs will feel differently, but I really don't think that there's any point in adding anything like this to Phobos. We already have ElementEncodingType.

[jmdavis]

@dsimcha Why do think that this is worth adding? It duplicates ElementEncodingType, and it has a horrible name. I was considering just closing it but figured that another Phobos dev should chime in first, not outright merge it.

[dsimcha]

Honestly, I have no idea how this slipped through the cracks in my mind. Merging it was definitely a mistake.

[dsimcha]

I don't have time right this second, but I'll make a pull request to revert this this evening if you don't beat me to it.

[jmdavis]

@dsimcha I just reverted it.

[denis-sh]

@jmdavis wrote:

I just reverted it.

In your local repo only. )
By the way, what is wrong with my first commit?

[jmdavis]

In your local repo only. )

Blast it. You're right. Fixing... Okay. It's been properly reverted now. Thanks for pointing that out. At least I accidentally commit to my repository rather than accidentally committing to the main one.

By the way, what is wrong with my first commit?

Glancing over it, it looks like it's probably fine, but I'm not going to take the time to cherry pick pull requests. If you have changes that you think are worth adding to Phobos separate from ArrayTarget, then please create a separate pull request for them.

[denis-sh]

The main purpose of this pull isn't addition of ArrayTarget template. It's replacing typeof(<type>.init[0]) with appropriate template. But it can't be done without addition or conclusion not to add ArrayTarget template.

[denis-sh]

I'm not going to take the time to cherry pick pull requests

Is it a joke? It takes less than a half of a minute, where is the trouble?

[jmdavis]

The main purpose of this pull isn't addition of ArrayTarget template. It's replacing typeof(<type>.init[0]) with appropriate template. But it can't be done without addition or conclusion not to add ArrayTarget template.

The title and description all talk about ArrayTarget, so the other changes come across as incidental.

Regardless, in general, none of us are going to be evaluating individual commits as to whether they should be merged or not. We may look at individual commits for large pull requests because it can make understanding the changes easier in those cases, but in general, we're going to just look at the diff as a whole and review the request as a whole. It's then either acceptable and is merged, needs work and the submitter is asked to make changes, or it's rejected. We don't take individual commits and merge them in because they seem good while the other others don't.

And honestly, don't expect much of anyone on the Phobos dev team to be doing much of anything with merging a pull request beyond hitting the merge button or not. Most of us learned git specifically because dmd, druntime, and Phobos were moved to github. We know what we need to in order to make that work, but most of us aren't all that skilled with git. I've used the cherry pick command all of maybe once, and I probably know git better than most of the Phobos devs, based on past questions and comments. It wouldn't surprise me if a number of us didn't even know about cherry picking at all.

I'll do it this once, but don't expect us to be doing it normally, and it'll probably take me longer to figure out how to do it than it would take you to create a new pull request. We're busy enough that dealing with pull requests in general proves to be problematic already (as the generally slow review and pull rate shows) without doing stuff like worrying about whether individual commits are worthwhile to be merged on their own.

[jmdavis]

Okay. The first commit with changes to use ElementEncodingType has been merged in.

[dnadlinger]

@jmdavis: By the way, many things involving maintaining a GitHub project are made almost ridiculously easy with the hub wrapper for Git. In this case, you could e.g. just have done git cherry-pick http://github.com/denis-sh/phobos/commit/ea6c9be (just copy URL from the commit link above).