Implement AA byPair.

[quickfur]

Resurrected from #574.

The basic idea is to provide this functionality in druntime, and if people insist, add a wrapper to Phobos that returns Tuples as people clamored for last time. Or just recommend users add this one-liner wrapper to their own code:

auto r = aa.byPair().map!(a => tuple(a.key, a.value));

then you'll a Tuple range out of it for no extra charge. :-)

[schveiguy]

See this comment, I think these should be easy to support

[bearophile]

I suggest to add the std.range.byTuple function too.

[bearophile]

I think this proposal allows to access fields with .key and .value field names:

foreach (t; [1: 2].byPair)
    writeln(t.key, " ", t.value);

But I think hypotetical future proposals for built-in tuples will not allow names for tuple fields. So at that point if byPair will be modified to return a built-in tuple, the user-code that uses the .key and .value field names will be broken. A more future-proof solution seems to allow only t[0] and t[1] access to the fields of a byPair pair.

[quickfur]

Why wouldn't future proposals for tuples not allow names for tuple fields? That would be a regression from the current Tuple type.

[quickfur]

@schveiguy I'm not sure how "easy" it is; sure it's easy to code, but it's as ugly as sin. The main reason is that the current implementation of front returns the original key/value by ref, and trying to shoehorn that into a Tuple-compatible type incurs additional copying overhead. This is one reason I do not agree with user code relying on explicit Tuple types. It leaks implementation details where it shouldn't matter.

Anyway, I've left .key and .value as-is for now, with the tuple-compatibility stuff off in its own ugly corner. I can't say I'm proud of this code at all. :-(

[bearophile]

@quickfur: I am not sure, but I think the current Tuple proposal+patch by Kenji (DIP32) doesn't include field names. And many built-in tuple implementations (like Haskell or Python ones) don't have such field names. Usually tuples with field names are named records...

[MartinNowak]

Yeah, please remove the .key and .value access.

[schveiguy]

Yeah, please remove the .key and .value access.

And how do you get ref access to the value if you return a tuple only? I think we are forgetting the power of D features.

[schveiguy]

I think these are redundant, I'm not sure how this compiles.

[quickfur]

Oops, mis-copy-n-pasted. Fixed.

[quickfur]

I would get rid of key and value if there was a way to overload opIndex with different return types, but currently there isn't.

[quickfur]

Alternatively, if there was a way to have a tuple of refs...

[schveiguy]

Yes, this is ugly. But it won't be when we can return tuples from functions (I hope?)

[schveiguy]

I found a better way to do this. Instead of using key and value as field names, use the tuple as the field, and opDispatch the key and value.

Note that this means the tuple must be a tuple of pointers. An example I whipped up (updated):

struct S(T...)
{
    T expand;
    ref opDispatch(string s)() if(s == "key") {return *expand[0];}
    ref opDispatch(string s)() if(s == "value") {return *expand[1];}
    alias expand this;
}

void main()
{
    int a;
    string b;
    S!(int*, string*) s;
    s[0] = &a;
    s[1] = &b;
    a = 1;
    assert(s.key == 1);
    s.key = 2;
    assert(a == 2);
    b = "hi";
    assert(s.value == "hi");
    s.value = "there";
    assert(b == "there");
}

[quickfur]

This is much better, yes. However, it fails the requirement that is(typeof(aa.byPair().front[0]) == Key) because the type will be Key* now. :-(

[schveiguy]

I'm OK with that ;) I'm sure others aren't though.

[schveiguy]

if there was a way to overload opIndex with different return types,

This gave me a great idea, will post in NG.

[mihails-strasuns]

I am strictly in favor of returning struct instance with .key / .value fields. One that wants tuple can still get it by using .tupleof - but this mess of a feature is simply not clean enough to casually use it in stable API. A lot needs to be changed / added before it becomes feasible.

[quickfur]

This is getting really annoying... I think we should just forget about tuple support in druntime, it's completely broken anyway, we should just rename this method byKeyValue, and leave byPair to Phobos where it belongs. I'm sick and tired of this stalemate. Either that, or I'll just close this PR and let somebody else take over. We still haven't made any progress after all this time, and I've stopped caring.

[mihails-strasuns]

As I have already mentioned in NG I have no idea where this push for tuples comes from and think it is a bad idea.

[schveiguy]

I agree with @Dicebot. Let's move forward with the byKeyValue idea. We can rename/add it later if tuple support becomes possible.

The idea that you can't range over an AA with both key and value when both are imminently accessible is ridiculous.

This is getting really annoying...

Sorry about the push for tuples, I didn't realize how messy it currently is, I thought it would be easier.

[quickfur]

Renamed to byKeyValue. I'll write up a byPair implementation for Phobos later.

[schveiguy]

LGTM.

Can you change commit comment to "Implement AA byKeyValue"?

Also, do you want to write an update to the online docs (apologies if you have already, I don't get notified of those)?

We should wait until @Dicebot and @MartinNowak examine before pulling. This is going to be a huge improvement, but obviously should be done with care.

[ghost]

LGTM. Seems simple enough not to cause any worries.

[quickfur]

Reworded.

I haven't written up the doc changes yet; will submit it later. Thanks for the reminder, though, I totally forgot we need to update the docs!

[quickfur]

Here's the doc pull: dlang/dlang.org#743

[MartinNowak]

Auto-merge toggled on

[bearophile]

If I compile this:

void main() {
    import std.stdio;
    auto aa = [1:2, 3:4];
    foreach (pair; aa.byKeyValue)
        pair.writeln;
}

I get:

C:\dmd2\src\druntime\import\object.di(555,28): Error: undefined identifier p
test.d(5,22): Error: template instance object.byKeyValue!(int[int], int, int) error instantiating

And isn't this:

auto byKeyValue(Value, Key)(Value[Key] aa) pure nothrow @nogc @property

Better than:

auto byKeyValue(T : Value[Key], Value, Key)(T aa) pure nothrow @nogc @property

[yebblies]

C:\dmd2\src\druntime\import\object.di(555,28): Error: undefined identifier p

Wow. The dangers of copy+paste. Wasn't someone working on generating object.di automatically?

[quickfur]

Fixed: #1077

[bearophile]

A problem with a constant key-value pair:

void main() {
    auto aa = [1:2, 3:4];
    foreach (const t; aa.byKeyValue) {
        auto k = t.key;
        auto v = t.value;
    }
}

test.d(4,18): Error: mutable method object.byKeyValue!(int[int], int, int).byKeyValue.Result.front.Pair.key is not callable using a const object
test.d(5,18): Error: mutable method object.byKeyValue!(int[int], int, int).byKeyValue.Result.front.Pair.value is not callable using a const object

[schveiguy]

A problem with a constant key-value pair:

accessors in Result should be inout. @quickfur I can handle this if you want.

[quickfur]

Done: #1079

[MartinNowak]

Was it impossible to make that compatible with the existing tuple expansion in foreach?
Because this doesn't work currently.

foreach (k, v; aa.byKeyValue.filter!pred)
{}

[schveiguy]

The consensus was not to invent a tuple that doesn't pass std.traits.isTuple. So it could be done, but probably not without issues.

You can wrap the result in a map!(a => tuple(a.key, a.value)) to get the behavior you want.

[quickfur]

@MartinNowak Try this instead:

import std.array : byPair;
foreach (k, v; aa.byPair.filter!pred) { ... }

[John-Colvin]

Is there a good reason why .length wasn't included?

[quickfur]

I just didn't think about it at the time. Please file an enhancement in bugzilla.

[quickfur]

Although on second thoughts, we might have trouble with .length if somebody modifies the AA while iterating over it...

[schveiguy]

Yeah, keeping track of length means saving an arbitrary integer to go along with the range to count down as you pop front. For the times it would be used, I think it's not worth the added footprint.

we might have trouble with .length if somebody modifies the AA while iterating

We don't need to worry about that, it's an invalid range at that point.