Skip to content

Add AliasSeqOf to std.meta#3785

Merged
Hackerpilot merged 1 commit intodlang:masterfrom
economicmodeling:add_AliasSeqOf
Nov 2, 2015
Merged

Add AliasSeqOf to std.meta#3785
Hackerpilot merged 1 commit intodlang:masterfrom
economicmodeling:add_AliasSeqOf

Conversation

@jcrapuchettes
Copy link
Contributor

Add template that allows for conversion of arrays and input ranges to AliasSeqs.

@Hackerpilot
Copy link
Contributor

@Hackerpilot
Copy link
Contributor

Context:
#3369
#1472

@JakobOvrum
Copy link
Contributor

PR #1472 had an optimization that yields less template instantiations for arrays, which is something that actually can be applied to all ranges with length and slicing. Would this be worth adding?

edit:

The result cannot contain types; the name shouldn't be capitalized.

@dnadlinger
Copy link
Contributor

It might make sense compile time-wise to transform all other ranges to arrays first using CTFE (array), and only then going into the template part (O(N) vs. O(1) instantiations).

std/meta.d Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the call to array if AliasSeqOf takes any input range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question and I've now removed it.

@jcrapuchettes
Copy link
Contributor Author

After evaluating the initial implementation and the implementation from #1472, I found that the #1472 was significantly better. It was able to handle ranges and arrays greater than 250 elements. To that end, I've copied the implementation from #1472, modified it a little, and copied in its unit tests.

Thanks to @monarchdodra!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I and V will never refer to types, so they should not be capitalized.

@JakobOvrum
Copy link
Contributor

LGTM. I would have preferred toAliasSeq but I'm fine with the current name too.

@Hackerpilot
Copy link
Contributor

Auto-merge toggled on

Hackerpilot added a commit that referenced this pull request Nov 2, 2015
@Hackerpilot Hackerpilot merged commit 9fc48a6 into dlang:master Nov 2, 2015
@monarchdodra
Copy link
Collaborator

Awesome!

I guess I don't get a say in this anymore, but if I had to write my original implementation again, I'd only implement support for arrays. std.meta should not have a dependency on std.range, as range is more of a library concept than language concept (and meta is more of a language support module). That and having the caller do aliasSeqOf!(myRange.array()) is trivially easy.

I'm not sure how much we are still trying to untangle library dependencies?

@JakobOvrum
Copy link
Contributor

@monarchdodra, good point, I agree it's better to leave the conversion to array to the caller. Maybe file a PR? I don't think this is part of the new release.

@jcrapuchettes jcrapuchettes deleted the add_AliasSeqOf branch November 5, 2015 17:45
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