Skip to content

Add ArrayElementType template (fixes issue 5061) #776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

denis-sh
Copy link
Contributor

@denis-sh denis-sh commented Sep 7, 2012

This supersedes accidentally merged and then reverted Pull #664.
Issue 5061 - std.traits.arrayTarget

See pull 664 comments for information/opinions.

@shoo
Copy link
Collaborator

shoo commented Sep 7, 2012

Does not std.traits.ForeachType/std.range.ElementType fit the purpose?

@denis-sh
Copy link
Contributor Author

denis-sh commented Sep 7, 2012

@shoo, see pull 664 comments please.
And you just shown that this pull is needed because people not dealing with ranges doesn't fill the difference between ElementType and ElementEncodingType.

@jmdavis
Copy link
Member

jmdavis commented Sep 7, 2012

I still think that this is pointless code duplication. ElementEncodingType does this already. Yes, it's an annoyingly long name, but we already have it, and it works. This is effectively just an alias for it, which we generally try to avoid in Phobos. I'll let some other Phobos devs weigh in on it rather than simply closing it, but I'm definitely against this.

@alexrp
Copy link
Member

alexrp commented Sep 16, 2012

Is there any actual reason ElementEncodingType cannot be used instead?

@denis-sh
Copy link
Contributor Author

Is there any actual reason ElementEncodingType cannot be used instead?

My opinion is in pull 664 comments. I can only add this:
Why are there writeln, writef, and writefln is we already have write? And why there are free functions if we already have stdout.write? Same for read* functions.

@alexrp
Copy link
Member

alexrp commented Sep 16, 2012

But those functions are there to actually make code easier to write, i.e. you have to do fewer calls in your code. This just seems like (effectively) an alias of ElementEncodingType.

@denis-sh
Copy link
Contributor Author

But those functions are there to actually make code easier to write,

arrayTarget is here to make code easier to read (more self-documented).

i.e. you have to do fewer calls in your code.

I see same calls count in every case: writeln(str), write(str, "\n"), stdout.write(str, "\n").

This just seems like (effectively) an alias of ElementEncodingType.

More precisely it is ElementEncodingType!T if isArray!T.

@alexrp
Copy link
Member

alexrp commented Sep 16, 2012

arrayTarget is here to make code easier to read (more self-documented).

I don't know what that is...

I see same calls count in every case: writeln(str), write(str, "\n"), stdout.write(str, "\n").

Let's not get pedantic (where's writef?). I think you know what my point is: Simplifying typical programming tasks.

But I think I now see your point: You want this trait because it only allows passing array types. While I can't think of idiomatic D code that wouldn't allow both range and array types in templatized code, I suppose it is reasonable to have this trait here for consistency if nothing else.

@denis-sh
Copy link
Contributor Author

I don't know what that is...
You want this trait because it only allows passing array types

Template code is rather difficult in D and it often not obvious what type can be used in particular place. And if you see ArrayElementType!T you understand that T is an array. That is what I call more self-documented code.

While I can't think of idiomatic D code that wouldn't allow both range and array types in templatized code

See this pull diff for examples in Phobos.

@andralex
Copy link
Member

I appreciate the arguments in favor of adding an ArrayElementType. However, ElementEncodingType is adequate (albeit unintuitively named) and I couldn't get convinced by the arguments involving precedents. I will close this now. Thanks for the contribution, and let's focus on the more meaty ones.

@DmitryOlshansky
Copy link
Member

Do I get the timing right in the github logs?
Meaning that this one was closed 3 months ago and the 2 dependent pull were opened 2 months later (i.e. around a month ago)...
@denis-sh Did you specifically made you subsequent pulls depend on the already rejected one?
What was the point then?

@denis-sh
Copy link
Contributor Author

Did you specifically made you subsequent pulls depend on the already rejected one?

Yes.

What was the point then?

As I disagree with rejecting of this pull and I always use ArrayElementType in my code and don't want to make my code uglier and less readable for phobos pulls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants