Skip to content

Add std.meta.applyLeft and std.meta.applyRight#3798

Merged
quickfur merged 1 commit intodlang:masterfrom
JakobOvrum:std_meta_apply
Feb 17, 2016
Merged

Add std.meta.applyLeft and std.meta.applyRight#3798
quickfur merged 1 commit intodlang:masterfrom
JakobOvrum:std_meta_apply

Conversation

@JakobOvrum
Copy link
Contributor

This should help eliminate a lot of tiny helper templates, including those that tend to be visible in template constraints despite not being documented anywhere.

Ping @Dicebot, @John-Colvin in case there's something about std.meta I should know about before adding new functionality to it.

std/meta.d Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, what I wanted to test here was type/symbol results. Will rewrite this test.

@mihails-strasuns
Copy link

Sorry, I have not paying attention to Phobos for quite a long time.

@JakobOvrum
Copy link
Contributor Author

The unit tests now also test alias results.

@JackStouffer
Copy link
Contributor

LGTM

std/meta.d Outdated
*/
template applyLeft(alias Template, args...)
{
static if(args.length)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: space between if and (.

@quickfur
Copy link
Member

Other than that, LGTM.

@John-Colvin
Copy link
Contributor

LGTM, not sure why I never thought to write these myself.

@John-Colvin
Copy link
Contributor

@JakobOvrum any chance you could fix those missing spaces? I'd love to see this merged.

@JakobOvrum
Copy link
Contributor Author

Fixed the spaces, but of course anyone with merge rights could just merge and fix such tiny issues themselves to speed up the process.

@quickfur
Copy link
Member

Auto-merge toggled on

quickfur pushed a commit that referenced this pull request Feb 17, 2016
Add std.meta.applyLeft and std.meta.applyRight
@quickfur quickfur merged commit 25aae21 into dlang:master Feb 17, 2016
@quickfur
Copy link
Member

Let's just get the queue moving again, and leave minor improvements to followup PRs.

@dnadlinger
Copy link
Contributor

These should have been CamelCased because they return templates, not values.

Can we please get this fixed before the release?

@dnadlinger
Copy link
Contributor

dnadlinger commented May 28, 2016

This shouldn't have been merged in the first place. Not only was there the issue of following the naming conventions; as it turns out, the implementation is woefully under-tested and unsurprisingly also broken. See, for example, #4374, or the fact the unit-tests don't seem to cover the zero-argument case (although it doesn't seem that it would need to be treated differently in the first place, except maybe as an exercise in premature optimisation).

Having ostensibly trivial helpers such as this in Phobos only makes sense if they actually work reliably in all cases.

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