Skip to content

Add std.typecons.Tuple.rename #4043

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

Merged
merged 8 commits into from
Sep 20, 2016
Merged

Add std.typecons.Tuple.rename #4043

merged 8 commits into from
Sep 20, 2016

Conversation

John-Colvin
Copy link
Contributor

Particularly useful to map on to a range of tuples generated from e.g. zip or cartesianProduct.

@JackStouffer
Copy link
Contributor

Would this be considered an equivalent to Python's namedtuple?

@John-Colvin
Copy link
Contributor Author

@JackStouffer Not quite. This is just a convenience function for renaming tuples. std.typecons.Tuple already contains the functionality of namedTuple

* there are members of `t`.
*/
template named(names ...)
if (allSatisfy!(isSomeString, staticMap!(TypeOf, names)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does typeof(names) work instead of staticMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks for the hint, fixed.

auto t2Named = t2.named!(["a": "b", "b": "c"]);
assert(t2Named.b == 3);
assert(t2Named.c == 4);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: afaik this empty line should be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done (and another one in the test below)

@John-Colvin John-Colvin force-pushed the named branch 3 times, most recently from 9eddb86 to f5a66d9 Compare March 24, 2016 17:33
@quickfur
Copy link
Member

ping @andralex
New addition, needs approval. Thanks!

@JakobOvrum
Copy link
Contributor

This doesn't seem that different from tuple!("foo", "bar")(t.expand)?

@wilzbach
Copy link
Contributor

wilzbach commented May 9, 2016

This doesn't seem that different from tuple!("foo", "bar")(t.expand)?

Ping @John-Colvin. Did you read this comment?

@andralex
Copy link
Member

I approve with two requests:

  • Make it a member of Tuple, it doesn't apply to anything. It would simplify implementation a tad.
  • Have it return a ref to the original object, it's more flexible.

@dlang-bot
Copy link
Contributor

@John-Colvin, thanks for your PR! By analyzing the annotation information on this pull request, we identified @9rnsr, @sinfu and @andralex to be potential reviewers. @9rnsr: The PR was automatically assigned to you, please reassign it if you were identified mistakenly.

(The DLang Bot is under development. If you experience any issues, please open an issue at its repo.)

@John-Colvin
Copy link
Contributor Author

@JakobOvrum in the most basic case, yes. But if you look at the examples you'll see it makes things like

  • renaming a single member which leaving the others, or
  • removing a name

much easier.

@codecov-io
Copy link

codecov-io commented Aug 22, 2016

Current coverage is 88.84% (diff: 97.26%)

Merging #4043 into master will increase coverage by 0.23%

@@             master      #4043   diff @@
==========================================
  Files           124        121      -3   
  Lines         77300      74357   -2943   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
- Hits          68495      66060   -2435   
+ Misses         8805       8297    -508   
  Partials          0          0           

Powered by Codecov. Last update 76cd4bd...84f9f95

@Cauterite
Copy link
Contributor

May I ask why the template is called named instead of something like renameFields ?

@John-Colvin
Copy link
Contributor Author

@Cauterite I think I'm going to rename it as I change it to a member of Tuple, as per @andralex's advice. renameFields seems like a reasonable choice but I haven't thought about it much.

@Cauterite
Copy link
Contributor

Oh, no problem then.

@John-Colvin John-Colvin changed the title Add std.typecons.named Add std.typecons.Tuple.rename Aug 22, 2016
@John-Colvin
Copy link
Contributor Author

@andralex (or anyone else), I'm struggling to work out how to make this return by ref.

@andralex
Copy link
Member

@John-Colvin use a trusted cast to pointer to your target type, and return *that. Makes sense?

@John-Colvin
Copy link
Contributor Author

@andralex yes, that works thanks.

I just remembered why I originally made it a free function:

zip(iota(10), iota(10, 20)).map!(rename!("a", "b"))

instead of now:

zip(iota(10), iota(10, 20)).map!(t => t.rename!("a", "b"))

I guess it's not really an important difference.

@John-Colvin
Copy link
Contributor Author

John-Colvin commented Aug 23, 2016

So I added some extra tests and found what seems to be a dmd bug: https://issues.dlang.org/show_bug.cgi?id=16418

I have a workaround for it in this case, see 40162fc

It seems that @dlang-bot thinks I'm posting fix because I mentioned it in the commit message :/

@lodo1995
Copy link
Contributor

lodo1995 commented Aug 24, 2016

It seems that @dlang-bot thinks I'm posting fix because I mentioned it in the commit message :/

No, the fix column in the bot message contains an X, not a V, so the bot is correctly considering this only a mention, not a fix.

@John-Colvin
Copy link
Contributor Author

anyone got any idea why the win 32 autotesters are failing?

@lodo1995
Copy link
Contributor

Error: more than 32767 symbols in object file

Seriously this limit?

Without really looking too much into this, it looks this PR is causing a growth in the number of symbols instantiated.

@wilzbach
Copy link
Contributor

Auto-merge toggled off

@wilzbach
Copy link
Contributor

wilzbach commented Aug 31, 2016

Auto-merge toggled off

It's been on the hot list of the auto tester for a while now and thus blocking other PRs.
(it's only temporarily until the windows problem is solved.)

anyone got any idea why the win 32 autotesters are failing?

You get:

Fatal Error: Out of memory

@John-Colvin
Copy link
Contributor Author

I don't have a windows machine to test on so this is a bit difficult for me to debug and fix.

@John-Colvin
Copy link
Contributor Author

I've moved the unittests to module scope in order to try and get the tests to pass on windows, but that means lack of documentation. Could do with some advice from compiler devs with windows knowledge here. @yebblies @9rnsr @WalterBright ??

@John-Colvin
Copy link
Contributor Author

Ok so it now passes on win32, that's good. But that leaves the documentation very much lacking. I guess I could duplicate the unittest inside the ddoc comments, but that's just.... ewww

@schveiguy
Copy link
Member

Related: http://wiki.dlang.org/DIP82

@andralex
Copy link
Member

@John-Colvin: could you please try the solution discussed in the forum?

@John-Colvin
Copy link
Contributor Author

Done

@andralex
Copy link
Member

Auto-merge toggled on

@John-Colvin
Copy link
Contributor Author

John-Colvin commented Sep 19, 2016

CircleCI failing because it can't find dub? And the auto tester is timing out in std.datetime...

@andralex andralex merged commit b97f005 into dlang:master Sep 20, 2016
@John-Colvin John-Colvin deleted the named branch September 20, 2016 08:47
@John-Colvin
Copy link
Contributor Author

Hooray! I wrote the first draft of this function almost 2 years ago, made the pull request 7 months ago, now we're finally here! I feel a blog post coming on...

Thanks to @andralex for getting back to this and helping make it phobos worthy, also thanks to everyone else involved for helping and nagging me to get it done.

@andralex
Copy link
Member

Thanks for pushing this through. A blog post sounds like a terrific idea.

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

Successfully merging this pull request may close these issues.