Issue 2486 - taking address of slice rvalue is valid

[9rnsr]

http://d.puremagic.com/issues/show_bug.cgi?id=2486

Requires: dlang/phobos#989

[MartinNowak]

Looks good, but given the impact this should go through a deprecation cycle.

[9rnsr]

Looks good, but given the impact this should go through a deprecation cycle.

Array slice should be essentially rvalue, so this is definitely a bug of current dmd - as same as "struct literal should be rvalue". For the bug, deprecation cycle is meaningless.

[9rnsr]

The merge has already happened when I had thought to do rebase. Thanks!

[ghost]

This is a big change. Here's some code which broke:

import std.conv;

void main()
{
    string s = "123";
    auto c = parse!uint(s[0 .. 3], 16);
}

We really can't casually merge pulls like this.

[dnadlinger]

@AndrejMitrovic: In general, I tend to agree. However, there is the question of how to determine the potential impact of the change. Kenji and Walter probably just couldn't think of any »sensible« piece of code that would be affected by this, and there isn't a really good way to find out about this other than having people test the change.

[ghost]

You could hang out in IRC and wait for someone to complain that DMD-head broke their code. ;)

And Martin warned it could break code: #1343 (comment)

[deadalnix]

Well, I should be infuriated, but I'm kind of used to it now. This indeed had to be fixed, but come on, not like this !

[deadalnix]

@klickverbot It break any code that pass a slice to a function that take a reference on a range. That sound convoluted said like that, but many function from phobos that operate on range take ref parameters, so it is fairly common, and slice is probably one of the most common range you'll find.

[andralex]

I'm thinking we could increase backward compatibility by having those functions accept auto ref.

[ghost]

I'm thinking we could increase backward compatibility by having those functions accept auto ref.

There could be many more functions. How do we decide which ones should or shouldn't take auto ref?

[ghost]

We better implement whatever we had in mind for auto ref before this becomes a problem for the next release.

AFAIK the plan was to turn an 'auto ref' function into a non-template which takes by ref, but where the compiler stores an r-value to a hidden l-value at the call site before the call.

[9rnsr]

@AndrejMitrovic If a breaking occurs in user code by this, it had essentially been meaningless or wrong. If a library utility rejects user code by this, it had not well designed and we should decide the utility design for rvalue slice.

Anyway, we must not add not-well-defined feature to language in order to avoid just one release pain.

[ghost]

If a breaking occurs in user code by this, it had essentially been meaningless or wrong.

That's not necessarily true. parse is a perfect example of this. It takes by ref so it can eat the elements in your array and leave you with the unparsed part. But it also allows you to call it with an rvalue slice if you only want the return value but you 1) don't want it to modify your original array or 2) you only want to pass a subset:

import std.conv;

void main()
{
    string s = "123 456 789";
    int first = parse!int(s);
    assert(first == 123);
    assert(s == " 456 789");

    int last = parse!int(s[5 .. $]);
    assert(s == " 456 789");
    assert(last == 789);
}

This is exactly how some user-code used to call parse. And in order for parse to continue working for both cases it must take its argument by auto ref.

But auto ref currently forces you to make the function a template. This is not a problem for parse in Phobos, but if library code has a similar function which has to be a non-templated function (either because it's an override or the user wants to take the address of it), then auto ref in its current form would break those requirements.

[9rnsr]

@AndrejMitrovic That's bad usage of std.conv.parse. parse intends to return consumed input through ref parameter. That is same as why std.conv.formattedWrite does not take non-ref parameter.

[ghost]

Yes it's a bad usage, but people have effectively used parse as if it was an auto ref function already. In other words, the accepts-invalid allowed people to write code like this.

Well anyway we can wait to see what the reaction will be after the release. I just wanted to mention it because I saw this bug in other people's code.

[9rnsr]

If would become really an issue, we can just add one more std.conv.parse overload for rvalue slice.

// Special case for rvalue slice, this is added for backward compatibility
Target parse(Target, SourceElem, Args...)(SourceElem[] s, Args args)
{
    return parse!Target(s, args);  // forwarding to other parse functions
}

unittest
{
    string s = "123 456 789";
    int last = parse!int(s[0 .. $]);
    assert(s == "123 456 789");
    assert(last == 123);
}