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 issue 17711: std.array.byPair ought to work with const AA's. #5668

Merged
merged 1 commit into from
Aug 2, 2017

Conversation

quickfur
Copy link
Member

@quickfur quickfur commented Aug 1, 2017

There's nothing about the implementation of byPair that precludes it from being used with const AA's, other than the way it's declared.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @quickfur!

Bugzilla references

Auto-close Bugzilla Description
17711 std.array.byPair should be usable with const AA

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Nice!

std/array.d Outdated
int[string] aa;
}
A a;
static assert(is(typeof(a)));
Copy link
Member

Choose a reason for hiding this comment

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

Hm... I'm not sure you need a special assert for this as you just declared it 😉

I'm going to go out on a limb, and say we don't need this extra test.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean the entire class definition? I'd tend to agree... I just thought it was extra insurance in case something else about the user's code may cause things to break.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think you can nix the whole class definition, it isn't adding any clarity (and it also adds a couple of uncovered lines since you don't actually run it).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

LGTM

@dlang-bot dlang-bot merged commit e4e858b into dlang:master Aug 2, 2017
@quickfur quickfur deleted the issue17711 branch August 2, 2017 16:08
@quickfur
Copy link
Member Author

quickfur commented Aug 2, 2017

Thanks, guys!

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.

5 participants