Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix - core.stdcpp.string.basic_string does not implement opEquals #3254

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented Oct 27, 2020

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
21344 normal core.stdcpp.string.basic_string does not implement opEquals

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + druntime#3254"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Oct 27, 2020
@n8sh n8sh force-pushed the issue-21344 branch 5 times, most recently from b803960 to ed18b43 Compare October 27, 2020 23:10
bool opCmp(scope const T[] rhs) const pure nothrow @safe { return __cmp(as_array, rhs); }

/// Hash to allow `basic_string`s to be used as keys for built-in associative arrays.
/// **The result will generally not be the same as C++ `std::hash<std::basic_string<T>>`.**
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should work this so that the result is the same as std::hash, although this is good for now.

/// ditto
bool opEquals(basic_string s) const { return as_array == rhs.as_array; }
/// ditto
bool opEquals(scope const T[] s) const pure nothrow @safe { return as_array == s; }
Copy link
Contributor

@TurkeyMan TurkeyMan Oct 28, 2020

Choose a reason for hiding this comment

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

Do these 3 functions work? This looks like it's comparing the slices? Ie, is it the same ptr and length, not is the string equal?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup.

void main()
{
    auto s1 = "abc";
    auto s2 = "abc".idup;
    assert(s1 == s2); // Passes: equal.
    assert(s1.ptr !is s2.ptr); // Passes: pointers are not the same.
    assert(s1 !is s2); // Passes: the pointers are not the same.
}

s1 == s2 gets lowered to __equals(s1, s2).

// The scalar-only overload takes advantage of known properties of scalars to
// reduce template instantiation. This is expected to be the most common case.
bool __equals(T1, T2)(scope const T1[] lhs, scope const T2[] rhs)
@nogc nothrow pure @trusted
if (__traits(isScalar, T1) && __traits(isScalar, T2))
{
if (lhs.length != rhs.length)
return false;
static if (T1.sizeof == T2.sizeof
// Signedness needs to match for types that promote to int.
// (Actually it would be okay to memcmp bool[] and byte[] but that is
// probably too uncommon to be worth checking for.)
&& (T1.sizeof >= 4 || __traits(isUnsigned, T1) == __traits(isUnsigned, T2))
&& !__traits(isFloating, T1) && !__traits(isFloating, T2))
{
if (!__ctfe)
{
// This would improperly allow equality of integers and pointers
// but the CTFE branch will stop this function from compiling then.
import core.stdc.string : memcmp;
return lhs.length == 0 ||
0 == memcmp(cast(const void*) lhs.ptr, cast(const void*) rhs.ptr, lhs.length * T1.sizeof);
}
}
foreach (const i; 0 .. lhs.length)
if (lhs.ptr[i] != rhs.ptr[i])
return false;
return true;
}

Copy link
Contributor

@TurkeyMan TurkeyMan Oct 28, 2020

Choose a reason for hiding this comment

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

Ummm... really?
Why would the slice comparison compare the contents element-wise? I thought s1[] == s2[] should be required to do that...
Man, apparently I haven't written much D code recently! O_O

src/core/stdcpp/string.d Outdated Show resolved Hide resolved
@TurkeyMan
Copy link
Contributor

TurkeyMan commented Oct 28, 2020

I actually had a good reason for these not existing originally... although I can't remember what it was.
I think I might have intended to do this: myString[] == myOtherString[] for comparisons.
...why did I think that? I'm sure I had a good reason...

@n8sh n8sh force-pushed the issue-21344 branch 2 times, most recently from 7ce191c to 8c9933d Compare October 28, 2020 04:43
@dlang-bot dlang-bot merged commit 278daf4 into dlang:stable Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants