String overloads by AlexGhiondea · Pull Request #7621 · dotnet/coreclr

AlexGhiondea · 2016-10-14T16:30:35Z

This PR addresses dotnet/corefx#5552 and was create to finish up PR #2945

…ditional overload signatures for string.Join to accept char separator. Since the implementation of: ```c# Join(String separator, String[] value, int startIndex, int count) ``` relies on `UnSafeCharBuffer.AppendString(string)` (which is not exposed to user land), I have added a char substitute for same. Also removed an additional check for `separator==null`, which wasn't present in `Join(String separator, IEnumerable<String> values)` either. `StringBuilder.Append` and `UnSafeCharBuffer.AppendString` both handle this case explicitly.

AlexGhiondea · 2016-10-14T16:31:02Z

This PR replaces #2945

AlexGhiondea · 2016-10-14T16:34:04Z

+        // Joins a string IEnumerable together as one string with a char separator between each original string.
+        //
+        [ComVisible(false)]
+        public static String Join(Char separator, IEnumerable<String> values) {


@terrajobst @weshaggard @stephentoub - Do we still want to keep this overload?

Why wouldn't we keep it?

oh I see @stephentoub comment on the old PR. I agree with @stephentoub that it isn't really needed unless we see some sort of other benefit.

justinvp · 2016-10-14T17:56:50Z

I don't know how folks feel about using unsafe code here, but in lieu of Span<char>, the implementations between the string and char separator overloads could be shared by doing something like:

public static unsafe string Join<T>(string separator, IEnumerable<T> values)
{
    fixed (char* pSeparator = separator)
    {
        int separatorLength = separator == null ? 0 : separator.Length;
        return Join(pSeparator, separatorLength, values);
    }
}

public static unsafe string Join<T>(char separator, IEnumerable<T> values)
{
    return Join(&separator, 1, values);
}

private static unsafe string Join<T>(char* separator, int separatorLength, IEnumerable<T> values)
{
    // Actual shared implementation here.
    // Uses the existing `StringBuilder.Append(char*,int)` to append the separator.
}

And done similarly for the other overloads.

Which is similar to how the the new string.Split overloads were implemented.

AlexGhiondea · 2016-10-15T00:06:42Z

@dotnet-bot please test CentOS7.1 x64 Debug Build and Test

AlexGhiondea · 2016-10-15T00:06:57Z

@dotnet-bot please test Linux ARM Emulator Cross Release Build

weshaggard · 2016-10-15T00:16:53Z

-
+
+        [System.Security.SecuritySafeCritical]  // auto-generated
+        public void AppendChar( char charToAppend ) {


nit why the space after ( and before )?

jamesqo · 2016-10-16T00:18:52Z

        }

+        // Joins an array of strings together as one string with a char separator between each original string.
+        //


Nit: Remove dead //s

jamesqo

There's a bug in the Join(char, object[]) overload where we return null if the 1st object is null. Also, I think it would be nice if you were able to format some of the code to the corefx coding conventions for better readability.

jamesqo · 2016-10-16T00:19:22Z


+        // Joins an array of strings together as one string with a char separator between each original string.
+        //
+        public static String Join(Char separator, params String[] value) {


Nit: Use string, char, and string.

Also put the brace on a newline.

jamesqo · 2016-10-16T00:19:49Z

+        //
+        public static String Join(Char separator, params String[] value) {
+            if (value == null)
+                throw new ArgumentNullException("value");


Nit: put braces here.

jamesqo · 2016-10-16T00:19:59Z

+        }
+
+        // Joins an object array of strings together as one string with a string separator between each original string.
+        //


Nit: dead // (same in other places)

jamesqo · 2016-10-16T00:20:32Z


+        // Joins an array of objects together as one string with a char separator between each original string.
+        //
+        [ComVisible(false)]


Are the ComVisibles still necessary?

jamesqo · 2016-10-16T00:21:00Z

+        // Joins an array of objects together as one string with a char separator between each original string.
+        //
+        [ComVisible(false)]
+        public static String Join(Char separator, params Object[] values) {


Nit: string, char, object. (same in other places)

jamesqo · 2016-10-16T00:21:10Z

+        [ComVisible(false)]
+        public static String Join(Char separator, params Object[] values) {
+            if (values == null)
+                throw new ArgumentNullException("values");


Nit: add braces. (same in other places)

jamesqo · 2016-10-16T00:22:40Z

+            if (values == null)
+                throw new ArgumentNullException("values");
+
+            if (values.Length == 0 || values[0] == null)


We don't want to propogate the bug where string.Join(string, object[]) returns null if the first item is null (even if the object has some non-null items) to more places.

jamesqo · 2016-10-16T00:23:21Z

+            Contract.Ensures(Contract.Result<String>() != null);
+            Contract.EndContractBlock();
+
+            using(IEnumerator<T> en = values.GetEnumerator())


Nit: Spacing

jamesqo · 2016-10-16T00:27:43Z

+            Contract.Ensures(Contract.Result<String>() != null);
+            Contract.EndContractBlock();
+
+            using(IEnumerator<String> en = values.GetEnumerator()) {


Nit: spacing, String.

jamesqo · 2016-10-16T00:28:52Z

+        [System.Security.SecuritySafeCritical]  // auto-generated
+        public void AppendChar( char charToAppend ) {
+            if ( (m_totalSize - m_length) < 1 ) {
+                throw new IndexOutOfRangeException();


We should probably look into changing this method (probably delete the class entirely) afterwards. In its current state this does not look like it will be inlined.

AlexGhiondea · 2016-10-17T17:20:55Z

It looks like this PR is not yet ready to be merged in.

There are still questions around the actual API surface area and the implementation needs more work.

I have marked the original issue as 'up for grabs' and I will close this PR for now.

Peter Jas and others added 2 commits October 13, 2016 20:32

Address minor code review feedback.

59f0b59

dnfclas added the cla-already-signed label Oct 14, 2016

AlexGhiondea added the api addition label Oct 14, 2016

AlexGhiondea added this to the Future milestone Oct 14, 2016

AlexGhiondea self-assigned this Oct 14, 2016

AlexGhiondea mentioned this pull request Oct 14, 2016

Adds String.Join overload to accept char separator #2945

Closed

AlexGhiondea commented Oct 14, 2016

View reviewed changes

weshaggard reviewed Oct 15, 2016

View reviewed changes

jamesqo reviewed Oct 16, 2016

View reviewed changes

jamesqo suggested changes Oct 16, 2016

View reviewed changes

AlexGhiondea closed this Oct 17, 2016

AlexGhiondea reopened this Oct 20, 2016

dnfclas added the cla-already-signed label Oct 20, 2016

AlexGhiondea closed this Oct 20, 2016

jamesqo mentioned this pull request Nov 1, 2016

Add String.Join overloads accepting a char separator #7942

Merged

karelz added enhancement Product code improvement that does NOT require public API changes/additions and removed api addition labels Mar 17, 2017

AlexGhiondea mentioned this pull request Jan 31, 2020

Add tests for new String.Join overloads dotnet/runtime#18962

Closed



		[System.Security.SecuritySafeCritical] // auto-generated
		public void AppendChar( char charToAppend ) {

Conversation

AlexGhiondea commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexGhiondea commented Oct 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinvp commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexGhiondea commented Oct 15, 2016

Uh oh!

AlexGhiondea commented Oct 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesqo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexGhiondea commented Oct 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AlexGhiondea commented Oct 14, 2016 •

edited

Loading

justinvp commented Oct 14, 2016 •

edited

Loading