Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Move S.Buffers and S.IO sources shared with corert into shared folder#9778

Merged
jkotas merged 5 commits into
dotnet:masterfrom
alexperovich:moveToShared
Feb 25, 2017
Merged

Move S.Buffers and S.IO sources shared with corert into shared folder#9778
jkotas merged 5 commits into
dotnet:masterfrom
alexperovich:moveToShared

Conversation

@alexperovich
Copy link
Copy Markdown
Member

This is the first set of changes to move common sources shared with corert into a shared folder.

Copy link
Copy Markdown
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM modulo a few comments

/// <summary>Prevents other processes from reading from or writing to the FileStream.</summary>
/// <param name="position">The beginning of the range to lock.</param>
/// <param name="length">The range to be locked.</param>
private void LockInternal(long position, long length)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this back? I though it got stripped because of it was not used anywhere.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These are in the corert sources. I assume they are still used in corert. If they aren't I will remove.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, looks like incorrectly resolved conflict with #9769

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Okay, will fix.

/// <summary>
/// Converts, resetting it, the last Win32 error into a corresponding <see cref="Exception"/> object, optionally
/// including the specified path in the error message.
/// </summary>
Copy link
Copy Markdown
Member

@jkotas jkotas Feb 24, 2017

Choose a reason for hiding this comment

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

Undo.

<MscorlibSources Include="$(MSBuildThisFileDirectory)\System\IO\FileStream.cs"/>
<MscorlibSources Include="$(MSBuildThisFileDirectory)\System\IO\Path.cs"/>
<MscorlibSources Include="$(MSBuildThisFileDirectory)\System\IO\PathInternal.cs"/>
<!--<MscorlibSources Include="$(MSBuildThisFileDirectory)\Microsoft\Win32\SafeHandles\SafeThreadPoolIOHandle.cs"/>-->
Copy link
Copy Markdown
Member

@jkotas jkotas Feb 24, 2017

Choose a reason for hiding this comment

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

This file should be deleted instead since it is not used in CoreCLR. Looks like it was added to CoreCLR by accident.

<TargetsOSX Condition="'$(TargetsOSX)' != 'true'">false</TargetsOSX>
</PropertyGroup>
<ItemGroup>
<MscorlibSources Include="$(MSBuildThisFileDirectory)\System\Buffers\*.cs"/>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to keep all files listed explicitly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

</PropertyGroup>
<ItemGroup>
<MscorlibSources Include="$(MSBuildThisFileDirectory)\System\Buffers\*.cs"/>
<MscorlibSources Include="$(MSBuildThisFileDirectory)\System\IO\Error.cs"/>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we not call it mscorlibSources because of it is not mscorlib anymore? The regular <Compile Include=... /> maybe best because of it is likely work the best with tooling.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was unsure if the MscorlibSources item was treated specially. If that isn't the case then I will just make these Compile items.

}

/// <summary>
/// Returns true if the path is too long
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I do not think we need any of these changes. They are unused code that was deleted in the CoreCLR version. The cleanup should be propagated to the CoreRT copy instead.

[Pure]
internal static class Error
{
internal static Exception GetStreamIsClosed()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This one is good. It is not used in CoreCLR right now, but it will be later once we get more files in sync.

/// <summary>
/// Returns a Win32 error code for the specified HRESULT if it came from FACILITY_WIN32
/// If not, returns the HRESULT unchanged
/// </summary>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Undo

/// <summary>
/// Returns true if the given StringBuilder starts with the given value.
/// </summary>
/// <param name="value">The string to compare against the start of the StringBuilder.</param>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The changes in this file should be also undone.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The rest looks good.

@alexperovich
Copy link
Copy Markdown
Member Author

@dotnet-bot test Windows_NT arm Cross Release Build please

@jkotas jkotas merged commit 532c45a into dotnet:master Feb 25, 2017
@alexperovich alexperovich deleted the moveToShared branch February 27, 2017 20:52
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants