Skip to content
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

Fix type tuple #780

Closed
wants to merge 3 commits into from
Closed

Fix type tuple #780

wants to merge 3 commits into from

Conversation

denis-sh
Copy link
Contributor

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

Fixes TypeTuple part of Issue 4113.

Still not sure about properly names (except for expressionTuple which is titled in Tuples article)

Note:

View first commit diff to see std/{typetuple.d → generictuple.d} renaming+changes.

[EDITED]
As it shown in the second commit TypeTuple is used ~70% of generic tuple usage.

@jmdavis
Copy link
Member

jmdavis commented Sep 9, 2012

While most of us will agree that TypeTuple is a bad name, it would break way too much code to change it now. This is exactly the sort of change that Walter will have kittens over if we make it. I'm pretty sure that we're stuck with TypeTuple at this point.

@denis-sh
Copy link
Contributor Author

denis-sh commented Sep 9, 2012

This pull incorporates no breaking changes because of (just updated) "backward compatibility" commit.

@denis-sh
Copy link
Contributor Author

denis-sh commented Sep 9, 2012

I have spent really a lot of time trying to figure out why the hell is TypeTuple used as an expression tuple. We are forcing every (as stupid as me) newbie to spend his time unless we will finally have right names and docs.

This is exactly the sort of change that Walter will have kittens over if we make it. I'm pretty sure that we're stuck with TypeTuple at this point.

Why not to use regular deprecation process as usual?

@jmdavis
Copy link
Member

jmdavis commented Sep 9, 2012

You're basically trying to get rid of TypeTuple, which is asking people to change a lot of code. You either end up ultimately deprecating TypeTuple (meaning that lots of code must be changed or it will break, even if it doesn't break immediately), or you end up with multiple constructs which do the same thing. Neither is desirable.

The push at this point is to stop breaking code (including to stop deprecating anything). If Walter had his way, we would pretty much never deprecate anything or make any breaking changes for any reason, and he's becoming increasingly intolerant of such changes - especially if it's to rename something. And this would break a lot of code, even if it wasn't immediate breakage.

We'll see what other devs say, but I think that we're just plain stuck with TypeTuple at this point.

@denis-sh
Copy link
Contributor Author

denis-sh commented Sep 9, 2012

Just want to mention that every use of TypeTuple not as a typetuple is against documentation and confuses people who read such code.

@jmdavis
Copy link
Member

jmdavis commented Sep 9, 2012

If you want a change like this to be made, you're probably going to have to convince Andrei (who may have to convince Walter). I've pretty much used up all of my karma with regards to pushing through breaking changes, and I think that Walter is particularly sick of me making such changes, even if they ostensibly make the library better. And this is a huge breaking change at a time when we're trying to stop making breaking changes altogether.

Just want to mention that every use of TypeTuple not as a typetuple is against documentation and confuses people who read such code.

Then the solution is to improve the documentation, not to make breaking changes. The construct is just fine with how it works now and doesn't need to be changed. It's just that it has a bad name. Regardless of whether Walter would accept renaming TypeTuple, I see no reason to split it up like you're doing here.

@denis-sh
Copy link
Contributor Author

denis-sh commented Sep 9, 2012

You either end up ultimately deprecating TypeTuple (meaning that lots of code must be changed or it will break, even if it doesn't break immediately), or you end up with multiple constructs which do the same thing. Neither is desirable.

And I don't want to get rid of TypeTuple - I like it. Coexisting of GenericTuple, expressionTuple, and TypeTuple it the right way IMHO.
The reason is the same as for ArrayElementType template - self documenting. And this template division documents much more that ArrayElementType template. I'm sure in it because I just revised Phobos, made all the necessary changes, and fill the difference.

@denis-sh
Copy link
Contributor Author

denis-sh commented Sep 9, 2012

@jmdavis, thanks a lot for the comments. But as you mentioned we have to wait Andrey and Walter with this.
I understand that you all are more experienced than me and I can be completely wrong. But this like some other my pulls is a result of everyday's PITA because I think that something is terribly wrong with Phobos - so I just doing my best to fix it.

So I did my best, it's other's turn. Will it be accepted or not - I don't bother. I just want you to make the right choice.

@ntrel
Copy link
Contributor

ntrel commented Sep 10, 2012

If renaming TypeTuple is too much (and I don't know whether it is), perhaps we can salvage the expressionTuple template (and the phobos changes to use it). expressionTuple could reside in std.typetuple, providing an alternative to TypeTuple. TypeTuple would keep its existing meaning, but users could choose to use expressionTuple instead when appropriate. Code using expressionTuple would express the intent clearly and avoid the tortuous use of TypeTuple for a tuple with no types in it at all.

@denis-sh
Copy link
Contributor Author

Carefully rebased after std.typetuple changes.

Also added some numbers to confirm my position about the fact that TypeTuple is a common case and must not be deprecated.

And I want to mention that it's not my main reason, it's just to show how silly it sounds (IMHO) "rename TypeTuple to GenericTuple with its current behavior to avoid unnecessary code duplication". My reason as always is a self-documentation of templates.

@alexrp
Copy link
Member

alexrp commented Oct 31, 2012

I guess we can close this since #904 supersedes it. I'll review #904 later today.

@alexrp alexrp closed this Oct 31, 2012
@denis-sh
Copy link
Contributor Author

I guess we can close this since #904 supersedes it. I'll review #904 later today.

No it doesn't.

@alexrp
Copy link
Member

alexrp commented Oct 31, 2012

I see. I hadn't noticed the changes you'd made to keep TypeTuple around. Will reopen.

@alexrp alexrp reopened this Oct 31, 2012
@alexrp
Copy link
Member

alexrp commented Oct 31, 2012

I agree that the name TypeTuple is misleading. I don't have any strong feelings about this rename since I admittedly don't use this module much. IOW, I would not be opposed to the rename, so long as the old module is kept for backwards compatibility.

@andralex @WalterBright I think we just need your opinions now.

Git note:
File renaming and creating a new file with same name must be a separated into different commits to allow rebasing this branch and viewing correct diff because git doesn’t explicitly track file movement. Note that final diff of a merge commit of this branch will still be inaccurate just like an automatic rebase of another branch to this commit.
All 388 usings of `TypeTuple` in Phobos are carefully inspected:

*  40 (~10%) times `expressionTuple` is used
*  76 (~20%) times `GenericTuple` is used
* 272 (~70%) times `TypeTuple` is used
@denis-sh
Copy link
Contributor Author

denis-sh commented Nov 1, 2012

Carefully rebased.

@jmdavis
Copy link
Member

jmdavis commented Nov 3, 2012

Andrei made some comments in the newsgroup semi-recently that indicated that he wouldn't be against renaming TypeTuple, given how bad a name it is, but I think that he's still going to have to chime in on this one given that it's doing more than simply renaming TypeTuple. I don't agree at all with trying to split TypeTuple into two like this is trying to do. Renaming is one thing. Redesigning is another, and I don't think that it needs a redesign.

And I confess that I don't think that GenericTuple is much better than TypeTuple as far as names go. It's still too much like Tuple. Something like ParamTuple or CTFETuple something would be better IMHO, and Timon was suggesting that it should be Sequence, since it's not really a tuple at all. But it represents a list of arguments or parameters for a function or template which are then used at compile time, not generic tuples. So, that being the case, something like ArgList or ParamList would be better IMHO. Whatever we pick needs to be clearly better than TypeTuple and not something which is easily confused with Tuple.

@John-Colvin
Copy link
Contributor

I think it's worth having a more open discussion in the newsgroup: http://forum.dlang.org/post/jagyupxiicevxrpnqewq@forum.dlang.org

@DmitryOlshansky
Copy link
Member

I'm on a trip over ~25 oldest pull request.

In this case it's abundantly clear that if something about std.typetuple is going to change it is a new std.meta module where we can put things "right". Too late to break old and tried std.typetuple.

See DIP54 and a discussion that still leaves a lot to debate:
http://wiki.dlang.org/DIP54
http://forum.dlang.org/thread/ggxgongvmrdfajtbphis@forum.dlang.org#post-mailman.137.1387767698.2938.digitalmars-d:40puremagic.com

Closing.

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