Add String.Join overloads accepting a char separator #7942

jamesqo · 2016-11-01T22:42:23Z

This PR adds string.Join overloads that take a char separator, which has been approved by dotnet/corefx#5552. It supplants #7621.

Please note that while I've verified the code I've wrote compiles, it's completely untested since I don't have a way to run tests on it. That will be done when the api is exposed in corefx.

Another note: I didn't add a Join(char, IEnumerable<string>) overload after seeing #7621 (comment) which referred to @stephentoub's comment at #2945 (comment).

cc @jkotas @stephentoub @AlexGhiondea @weshaggard @bbowyersmyth @ghost @hughbe

jamesqo · 2016-11-01T22:42:57Z

src/mscorlib/src/System/String.Manipulation.cs

@@ -541,6 +541,184 @@ public String Insert(int startIndex, String value)
            }
            return result;
        }
+
+        public static string Join(char separator, params string[] value)


I kept the parameter names the same as the string-based overloads, so this is value as opposed to values.

jamesqo · 2016-11-01T22:43:53Z

src/mscorlib/src/System/String.Manipulation.cs

+                throw new ArgumentNullException("values");
+            }
+
+            if (values.Length == 0)


I avoided duplicating the Join(string, object[]) bug where we return "" if the first item is null here.

@justinvp Unfortunately, this means that it's not possible to share code with the string overload here for now. Ongoing discussion @ https://github.com/dotnet/coreclr/issues/4506#issuecomment-258033970

You can still share the code - just need a bool argument on what the behavior in this case should be.

@jkotas, that seems kind of ugly to me. I think it would be better to simply leave things as-is for now, and when fixing the bug is approved then share the code.

jamesqo · 2016-11-01T22:46:49Z

src/mscorlib/src/System/String.Manipulation.cs

+                string currentValue = value[i];
+                if (currentValue != null)
+                {
+                    checked


Used checked inside the loop, instead of checking for overflow at the end. 3 2B sized strings for example could overflow enough that the total length goes back to a positive value, so the check seems fairly pointless.

This will throw OverflowException. Other Join overloads throw OutOfMemory exception in this case...

Hm, I can change it to do a totalLength < 0 check instead then.

jamesqo · 2016-11-01T22:48:23Z

src/mscorlib/src/System/String.Manipulation.cs

+
+            for (int i = startIndex, end = startIndex + count; i < end; i++)
+            {
+                // It's possible that another thread may have mutated the input array


I happened to see @stephentoub's PR a while ago at #4559, so I mimicked his logic here.

jamesqo · 2016-11-01T22:49:22Z

src/mscorlib/src/System/String.Manipulation.cs

+                if (i < end - 1)
+                {
+                    // Fill in the separator.
+                    fixed (char* pResult = &result.m_firstChar)


fixed may/may not have better performance outside the loop, but I think putting it here is best for readability.

Wouldn't a simple result[copiedLength] = separator be faster?

@jkotas result is a string, so using a set indexer won't work here.

jamesqo · 2016-11-01T22:49:44Z

src/mscorlib/src/System/String.Manipulation.cs

+            // something changed concurrently to mutate the input array: fall back to
+            // doing the concatenation again, but this time with a defensive copy. This
+            // fall back should be extremely rare.
+            return copiedLength == totalLength ? result : Concat((string[])value.Clone());


Logic copied from @stephentoub's PR.

hughbe · 2016-11-01T22:58:06Z

Nice! In the issue over in corefx I said I would add some tests and expose the implementation - I don't actually have time to do this anytime soon, so if you want you can take over

justinvp · 2016-11-02T00:02:14Z

src/mscorlib/src/System/String.Manipulation.cs

+        {
+            if (value == null)
+            {
+                throw new ArgumentNullException("value");


Nit: nameof (nameof was just deployed throughout mscorlib). Applies throughout.

justinvp · 2016-11-02T00:03:58Z

Any thoughts on sharing code with the string separator methods? #7621 (comment)

jamesqo · 2016-11-02T22:45:01Z

@hughbe Sure, I would be willing to add tests / expose the impl in corefx.

jamesqo · 2016-11-02T22:46:01Z

@justinvp Great idea! Esp. considering one of the Join overloads is generic it should help shave down on some jitted code bloat if an app uses both overloads with the same type.

justinvp · 2016-11-02T23:51:57Z

src/mscorlib/src/System/String.Manipulation.cs

-            Contract.EndContractBlock();
-
-            using (IEnumerator<T> en = values.GetEnumerator())
+            fixed (char* pSeparator = &separator.m_firstChar)


What if separator is null?

@justinvp Then pSeparator will be initialized to null. This is technically implementation-defined behavior of Roslyn, but it looks like they're planning to update the spec sometime.

On hindsight, the real issue here looks like separator.Length will nullref if it is null (edit: I just realized, m_firstChar as well)... I'll just add a separator = separator ?? string.Empty beforehand. Thanks for pointing this out.

jamesqo · 2016-11-03T00:43:01Z

src/mscorlib/src/System/UnSafeCharBuffer.cs

-    using System.Diagnostics;
-    using System.Diagnostics.Contracts;
-
-    unsafe internal struct UnSafeCharBuffer{


This file is no longer being used after the Join char/string impls were unified, so I deleted it.

jkotas · 2016-11-03T00:53:06Z

Explicit check for overflow may be a bit faster, like:

long totalSeparatorsLength = (long)(count - 1) * separatorLength;
if (totalSeparatorsLength > int.MaxValue)
   throw new OutOfMemoryException(); 
int totalLength = (int)separatorsLength;

jamesqo · 2016-11-03T01:04:31Z

@jkotas Nice idea. Updated the PR to match.

justinvp · 2016-11-03T01:41:24Z

src/mscorlib/src/System/String.Manipulation.cs

+            separator = separator ?? string.Empty;
+            fixed (char* pSeparator = &separator.m_firstChar)
+            {
+                // Defer argument validataion to the internal function


Nit: validataion -> validation

justinvp · 2016-11-03T01:44:26Z

src/mscorlib/src/System/String.Manipulation.cs

+            return Join(separator, value, 0, value.Length);
+        }
+
+        public static string Join(char separator, params object[] values)


When I added the new Split overloads, I made them all [ComVisible(false)]. I think we should do the same for these new Join overloads.

Ok edit: See @danmosemsft's comment

justinvp · 2016-11-03T01:45:35Z

src/mscorlib/src/System/String.Manipulation.cs

+        }
+
+        [ComVisible(false)]
+        public static String Join(String separator, IEnumerable<String> values) {


Since you changed String -> string above, maybe go ahead and do the same here.

I don't know why these overloads are diffing since I didn't change them... but since they are, I guess I might as well format them.

justinvp · 2016-11-03T01:45:59Z

src/mscorlib/src/System/String.Manipulation.cs

            if (values == null)
                throw new ArgumentNullException(nameof(values));
            Contract.Ensures(Contract.Result<String>() != null);
            Contract.EndContractBlock();

+            using(IEnumerator<String> en = values.GetEnumerator()) {


Nit: space between using and (

justinvp · 2016-11-03T01:49:07Z

src/mscorlib/src/System/String.Manipulation.cs

+            return StringBuilderCache.GetStringAndRelease(result);
+        }
+
+        public unsafe static string Join<T>(char separator, IEnumerable<T> values)


Should the public unsafe methods be annotated [SecuritySafeCritical] and private JoinCore annotated [SecurityCritical]?

If I understand @jkotas comment https://github.com/dotnet/corefx/issues/12592#issuecomment-253971384 they are not needed in this assembly as we don't build mscorlib for desktop and the mirror is switched off. @jkotas can confirm.

Same I think for [ComVisible]. https://github.com/dotnet/corefx/issues/12592#issuecomment-253972687

Ok, will not add unless someone says otherwise.

If I understand @jkotas comment dotnet/corefx#12592 (comment) they are not needed in this assembly as we don't build mscorlib for desktop and the mirror is switched off.

I realize that's the case for corefx. I asked because I'm not sure about coreclr given it appears the security annotations are still being maintained here (e.g. #7476). @jkotas should confirm.

It is not necessary to maintain these anymore.

AlexGhiondea · 2016-11-03T17:54:58Z

src/mscorlib/src/System/String.Manipulation.cs

+
+        // Joins an array of strings together as one string with a separator between each original string.
+        //
+        [System.Security.SecuritySafeCritical]  // auto-generated


I think you can remove the // auto-generated part of the comment.

@AlexGhiondea There appear to be lots of those comments throughout the repo; if such a change is made it should probably be done in bulk.

@jamesqo that would work. I was just thinking that since you are adding this method you can clean-up the comment. I am fine with it as-is :).

jamesqo · 2016-11-04T22:54:39Z

@jkotas, I think this is ready to be merged.

jkotas · 2016-11-06T23:49:59Z

@AlexGhiondea Could you please take care of this?

jamesqo · 2016-11-16T22:03:05Z

Looks like the string.Join(string,object[]) bug is no more, since #8114 has been merged. In a few days (after the corefx tests for that PR have been merged), I will update this PR again and introduce code sharing between the (char,object[]) & (string,object[]) overloads.

AlexGhiondea · 2016-11-21T18:46:08Z

@jamesqo could you rebase and update the PR so that we can merge this?
Thanks for working on it!

jamesqo · 2016-11-21T22:30:09Z

@AlexGhiondea Sure. This is ready for merge now, I believe.

AlexGhiondea

Thanks for taking care of this!

danmoseley · 2016-11-22T18:44:24Z

@stephentoub apparently has a couple comments b efore we merge this.

jamesqo · 2016-11-22T19:20:19Z

@danmosemsft @stephentoub What are they?

stephentoub · 2016-11-22T21:54:23Z

src/mscorlib/src/System/String.Manipulation.cs

+                    if (totalLength < 0) // Check for overflow
+                    {
+                        throw new OutOfMemoryException();
+                    }


It looks like the original author went out of his or her way to keep this check out of this loop. I assume this doesn't make a measurable impact, but have you checked?

@stephentoub The original version was incorrect in the first place. jointLength < 0 does not tell us whether anything overflowed b/c the int could have wrapped around to negative and passed 0 again, since we're dealing with a variable amount of strings. jointLength + 1 < 0 is also pointless.

You could check for overflow by doing something like this:

'If (totalLength + currentValue.Length < totalLength)'

@AlexGhiondea That would have to be in the loop as well; there is no difference from the version that is currently checked in.

@jamesqo yes, it should still be in the loop but that check would catch overflows even when the value overflows enough to come back into the positive number range.

@AlexGhiondea, a single int + int cannot overflow that much. Worst case scenario, totalLength and currentValue.Length are both int.MaxValue. Their sum is only -2. It's only when we multiply we run into problems.

@jamesqo ah yes -- I was thrown off by your comment above about the previous version of the code 😄

* Add String.Join overloads that accept char separators

…7942) * Add String.Join overloads that accept char separators Commit migrated from dotnet/coreclr@3f59c07

dnfclas added the cla-already-signed label Nov 1, 2016

jamesqo commented Nov 1, 2016

View reviewed changes

justinvp reviewed Nov 2, 2016

View reviewed changes

jamesqo commented Nov 3, 2016

View reviewed changes

jamesqo force-pushed the new-string-apis branch from f649e89 to 1abc87f Compare November 3, 2016 00:53

justinvp reviewed Nov 3, 2016

View reviewed changes

AlexGhiondea reviewed Nov 3, 2016

View reviewed changes

jkotas assigned AlexGhiondea Nov 6, 2016

jamesqo added 2 commits November 21, 2016 17:03

Add String.Join overloads that accept char separators

0439b29

Add new members to model.xml

e9923c7

jamesqo added 9 commits November 21, 2016 17:03

Code sharing for generic Join overloads

b1a33d1

Don't nullref if the separator is null

12ec865

Code sharing for Join accepting a string[],int,int

edaed6f

UnSafeCharBuffer is no longer being used, so delete it

8ded066

Use nameof

f9e57c2

Respond to PR feedback

91ee75d

Typo

5e9f536

Format IEnumerable<string> overload

c5c9f55

Format some more Join overloads

c60c141

jamesqo force-pushed the new-string-apis branch from 8f794cc to c60c141 Compare November 21, 2016 22:06

jamesqo changed the title ~~Add String.Join overloads accepting a char separator~~ [no merge] Add String.Join overloads accepting a char separator Nov 21, 2016

Share code for remaining Join overload.

6a80496

jamesqo changed the title ~~[no merge] Add String.Join overloads accepting a char separator~~ Add String.Join overloads accepting a char separator Nov 21, 2016

AlexGhiondea approved these changes Nov 21, 2016

View reviewed changes

stephentoub reviewed Nov 22, 2016

View reviewed changes

stephentoub approved these changes Nov 22, 2016

View reviewed changes

AlexGhiondea merged commit 3f59c07 into dotnet:master Nov 23, 2016

jamesqo deleted the new-string-apis branch November 23, 2016 23:41

This was referenced Nov 25, 2016

Port new string.Join overloads dotnet/corert#2268

Closed

Port new string.Join overloads from coreclr. dotnet/corert#2270

Merged

sergign60 pushed a commit to sergign60/coreclr that referenced this pull request Dec 2, 2016

Add String.Join overloads accepting a char separator (dotnet#7942)

6ce292f

* Add String.Join overloads that accept char separators

karelz modified the milestone: 2.0.0 Aug 28, 2017

GrabYourPitchforks mentioned this pull request Feb 13, 2020

Internal overflow bug in string.Join(string[]) when passing huge strings dotnet/runtime#5591

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Add String.Join overloads accepting a char separator (dotnet/coreclr#…

7b971ba

…7942) * Add String.Join overloads that accept char separators Commit migrated from dotnet/coreclr@3f59c07

Add String.Join overloads accepting a char separator #7942

Add String.Join overloads accepting a char separator #7942

Conversation

jamesqo commented Nov 1, 2016

jamesqo Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe commented Nov 1, 2016

Choose a reason for hiding this comment

justinvp commented Nov 2, 2016

jamesqo commented Nov 2, 2016

jamesqo commented Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

jamesqo Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Nov 3, 2016

jamesqo commented Nov 3, 2016

Choose a reason for hiding this comment

justinvp Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

jamesqo Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo commented Nov 4, 2016

jkotas commented Nov 6, 2016

jamesqo commented Nov 16, 2016

AlexGhiondea commented Nov 21, 2016

jamesqo commented Nov 21, 2016

AlexGhiondea left a comment

Choose a reason for hiding this comment

danmoseley commented Nov 22, 2016

jamesqo commented Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

jamesqo Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo Nov 1, 2016 •

edited

Loading

jamesqo Nov 3, 2016 •

edited

Loading

jamesqo commented Nov 2, 2016 •

edited

Loading

jamesqo Nov 3, 2016 •

edited

Loading

justinvp Nov 3, 2016 •

edited

Loading

jamesqo Nov 3, 2016 •

edited

Loading

jamesqo commented Nov 22, 2016 •

edited

Loading

jamesqo Nov 23, 2016 •

edited

Loading

jamesqo Nov 23, 2016 •

edited

Loading

jamesqo Nov 23, 2016 •

edited

Loading