Skip to content

Conversation

@schuetzm
Copy link
Contributor

Fixed the regression introduced in #3172

@schveiguy
Copy link
Member

Hm.. I think this is not what was asked for. The original code did not instantiate a new template per static array size. This is kind of a very tricky issue, because you want to use T[] not T[N], but T[] is a valid range, so it will match the first overload also.

The only good news here is that the template bloat should be minimal in terms of code size (the function should always inline), but it will hurt in the symbol table. If ever there was a case for @forceinline, this is it.

I'm OK with this change, the template bloat should not be that bad. If others feel differently, I'd love to hear another solution.

@schuetzm schuetzm force-pushed the indexof-fixed-size-array branch 2 times, most recently from 06e3ac8 to 3b386f8 Compare April 16, 2015 20:12
@schuetzm
Copy link
Contributor Author

I could factor the actual implementation out into a third, private template that is called by both of the others. But IMO it's not worth the trouble, and it would even make the common case worse, because it would instantiate two templates for each type.

std/string.d Outdated
Copy link
Member

Choose a reason for hiding this comment

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

use:

ref T[n] s

instead, otherwise the entire array gets copied to the stack!

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, good catch!

@schveiguy
Copy link
Member

But IMO it's not worth the trouble

yeah, agreed.

@schuetzm schuetzm force-pushed the indexof-fixed-size-array branch from 3b386f8 to 97dddde Compare April 16, 2015 20:39
@schveiguy
Copy link
Member

LGTM with latest changes.

@JakobOvrum
Copy link
Contributor

You'd think that having an overload receiving T[] would work properly as it is more specialized than just Range, but DMD doesn't implement it...

WalterBright added a commit that referenced this pull request Apr 17, 2015
Re-add overload for fixed-size arrays to `std.string.indexOf`
@WalterBright WalterBright merged commit 325c7b0 into dlang:master Apr 17, 2015
@schuetzm schuetzm deleted the indexof-fixed-size-array branch April 17, 2015 08:28
@JinShil
Copy link
Contributor

JinShil commented Jun 26, 2015

This pull request introduced a regression: https://issues.dlang.org/show_bug.cgi?id=14735

@schuetzm
Copy link
Contributor Author

Can someone figure this out? Removing the overload added in this PR makes it work, but this overload shouldn't match for the test case in the bug report, so I don't see how it could even have an effect. Is this a compiler bug?

@JinShil
Copy link
Contributor

JinShil commented Jun 27, 2015

The problem has something to do with the CaseSensitive argument. See the bug report for a test case.

schuetzm added a commit to schuetzm/phobos that referenced this pull request Jun 27, 2015
@MartinNowak
Copy link
Member

Was this the only regression regarding static arrays of newly rangified functions?

Copy link
Member

Choose a reason for hiding this comment

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

An optimization would be to use const(T)[], b/c of less template instantiations.

@MartinNowak
Copy link
Member

I think a better alternative would be to add explicit function overloads for const(char/wchar/dchar)[].
That makes sure the overload is more specialized, we can precompile the most common instantiations into libphobos, and it allows to coerce to const(char)[] to deduplicate all const variations of narrow strings.

void indexOf(Range)(Range s, in dchar c) @safe pure
{
    pragma(msg, "Range "~Range.stringof);
}

void indexOf(const(char)[] s, in dchar c) @safe pure
{
    pragma(msg, "const(char)[]");
    .indexOf!(const(char)[])(s, c);
}

unittest
{
    char[] ary;
    char[128] sary;
    indexOf(ary);
    indexOf(sary);
}

@MartinNowak
Copy link
Member

This is troublesome b/c of polysemeous string literals and conversion of other arguments.

void test(const(char)[] s, in dchar c) {}
void test(const(dchar)[] s, in dchar c) {}

void test()
{
    test("foo", 'a'); // 2nd arguments matches with conversion, so both overloads match equally well
}

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