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

Added new StaticTuple template and deprecated use of TypeTuple with non-... #1309

Closed
wants to merge 1 commit into from

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented May 27, 2013

...type parameters

http://forum.dlang.org/thread/hwlrptkobszameyvkgam@forum.dlang.org

TypeTuple!(int, "Hello") will now cause a deprecation warning
TypeTuple!(int, float) will work exactly as before

StaticTuple!(int, "Hello") is used for non-type tuples

StaticTuple!(int, float) == TypeTuple!(int, float)

Library code has been updated to use StaticTuple in the case that it depends on the undocumented feature of TypeTuple to store non-types.

For what it's worth, the only major uses of the non-type version of TypeTuple were in std.traits and std.typetuple itself, and of the rest (~30) the majority were used in unit test code.

@ghost
Copy link

ghost commented May 27, 2013

There's already #780

@dnadlinger
Copy link
Member

As much as I hate that not only deceptive, but also unwieldy name myself, there is no way we are going to be able to change it at this point. Even if it does increase client code readability, the amount of breakage caused is just too much compared to that (there were several NG discussions about this specific case already).

The only real way I see out of this situation is during introducing a modern std.meta, which would likely feature {Expr, Type}Seq constructs (under whatever name we'd settle on, this is just my personal preference) and gradually replace std.typetuple.

@dnadlinger dnadlinger closed this May 27, 2013
@ghost
Copy link

ghost commented May 27, 2013

Yeah totally agreed. If you want just a glimpse of how much code would change have a look at https://github.com/D-Programming-Language/phobos/pull/780/files

@deadalnix
Copy link
Contributor

This is why the thing is made a depreaction warning instead of a failure. Old should continue to work for a while.

@Diggsey
Copy link
Contributor Author

Diggsey commented May 27, 2013

Most of those changes are related to the module name, which I am not suggesting to change. I know exactly how much code has to change because I changed it in this commit and it was very little considering the size of phobos.

IMO you are far more likely to put people off using the language due to important language features such as tuples being incomprehensible rather than by a deprecation message that can be fixed for an arbitrarily large project in a few key-strokes (find and replace). Especially as the feature is undocumented anyway, and it is frequently mentioned in the documentation that TypeTuples are for storing Types.

Confusing people about TypeTuples that are not actually in any way TypeTuples is something that will stay with the language forever if something is not done about it. A deprecation message that can be fixed this easily is something that occurs once and there are absolutely no other consequences...

edit:
Also, even though it is deprecated, it doesn't ever have to be removed. All the benefits of its existence (logical naming convention, opening for other types of tuples, being able to annotate what the contents of a tuple should be, and compile time detection of invalid contents) still occur without TypeTuple ever being changed to only support types, indeed the only one that even requires a deprecation message is the compile time detection part.

I just don't see what the fuss is about, Walter has repeatedly said that "breaking" changes must be considered in terms of benefits compared to code breakage. In this case the actual amount of code not compiling as a result is zero since the only effect is a warning, and fixing said warning is as simple as it can possibly be. On the other hand the benefits are huge and will exist for the rest of time. Let's face it, do you know any major languages which still have any issues like this that haven't been addressed?

@dnadlinger
Copy link
Member

I closed the pull request simply because there is currently no consensus for such a change, see e.g. #780.

This is not to say that there is no merit to this pull request – as I said, I am personally in favor of such a change, or at least would have been for a long while. It's simply that leaving a pull request open that isn't going to be merged any time soon without a prior extended discussion on the forums (which I encourage you to take up!) only uses up auto tester time and clutters up the pull request list. This is especially bad because we aren't doing a great job in keeping up with Phobos pull requests right now anyway.

@dnadlinger dnadlinger reopened this May 27, 2013
@dnadlinger
Copy link
Member

Oh, odd – I must have somehow missed the "Tuple" thread on dm.D so far. If I had been aware that the topic has been brought up again, I wouldn't have closed the pull request until the discussion has fizzled out (or it has been merged), as that creates a fair bit of friction on its own. Anyway, the discussion should continue on the forums.

@Diggsey
Copy link
Contributor Author

Diggsey commented May 27, 2013

I created a separate thread for discussion as the original was technically about the documentation aspect and I don't want to hijack that:
http://forum.dlang.org/thread/hwlrptkobszameyvkgam@forum.dlang.org

andralex pushed a commit to andralex/phobos that referenced this pull request Jun 3, 2013
By fixing bug 9069 and 9035, T.init always returns rvalue and ref cannot receive it.
andralex pushed a commit to andralex/phobos that referenced this pull request Jun 3, 2013
@ghost
Copy link

ghost commented Oct 24, 2013

Until we have a consensus in the community I'm closing this, it's only going to keep breaking as new code uses TypeTuple in Phobos and the pull is out of date.

@ghost ghost closed this Oct 24, 2013
This pull request was closed.
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.

3 participants