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

CoreFX #24343 Vector Ctor using Span #16733

Merged
merged 3 commits into from
Mar 7, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
CoreFX #24343 Vector using Span
dotnet/corefx#24343
  • Loading branch information
WinCPP committed Mar 5, 2018
commit 52e95add0cc1eccc2120034d940f6cab77be4d56
1 change: 1 addition & 0 deletions src/mscorlib/System.Private.CoreLib.csproj
Original file line number Diff line number Diff line change
@@ -34,6 +34,7 @@
<SignAssembly>true</SignAssembly>
<DelaySign>true</DelaySign>
<DefineConstants>$(DefineConstants);CORECLR;_USE_NLS_PLUS_TABLE;RESOURCE_SATELLITE_CONFIG;CODE_ANALYSIS_BASELINE</DefineConstants>
<DefineConstants Condition="'$(TargetGroup)'=='netcoreapp' OR '$(TargetGroup)'=='uap' OR '$(TargetGroup)'=='uapaot'">$(DefineConstants);netcoreapp</DefineConstants>
Copy link
Member

Choose a reason for hiding this comment

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

This can defined unconditionally here. The condition is not necessary.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

<!-- We don't use any of MSBuild's resolution logic for resolving the framework, so just set these two properties to any folder that exists to skip
the GenerateReferenceAssemblyPaths task (not target) and to prevent it from outputting a warning (MSB3644). -->
<_TargetFrameworkDirectories>$(MSBuildThisFileDirectory)/Documentation</_TargetFrameworkDirectories>
4 changes: 3 additions & 1 deletion src/mscorlib/shared/System/Numerics/Vector.cs
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#if (!netfx && !netstandard)
#if netcoreapp
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we use the existing netstandard constant here (for instance just like https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/MemoryExtensions.cs#L9).

Copy link
Member

Choose a reason for hiding this comment

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

We discussed that here: #16733 (comment)

using Internal.Runtime.CompilerServices;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be under ifdef. Otherwise, it won't build once mirrored to CoreFX used to build the portable Vector version.

Copy link
Author

Choose a reason for hiding this comment

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

Mirroring as in, is there going to be an attempt later sometime to single-instance the copies of shared code base between CoreClr and CoreFx repos? In that case do you want me to add #ifdef netcoreapp in here with appropriate definition of netcoreapp in the csproj?

Copy link
Member

Choose a reason for hiding this comment

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

This single instancing is in place already. Once this PR is merged, there will be PR with your change created in corefx automatically: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/README.md

We use #if !netstandard in other similar places: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs#L11 . I would try to stick with that unless if does not work for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

In CoreFx, we include this file when building for netfx and netstandard. So I'm guessing !netstandard isn't going to be enough.

Also - the whole new constructor should probably be under the ifdef, so it doesn't exist in the netfx and netstandard versions.

Copy link
Author

@WinCPP WinCPP Mar 5, 2018

Choose a reason for hiding this comment

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

@eerhardt I again went through the csproj for CoreFx project and in there Vector.cs is included if not targetting netcoreapp. I am thoroughly confused when it comes to nomenclature such as netstandard vs netfx, netcoreapp, etc. So do you mean to say that I should put the using statement and entire new constructor under #if (!netfx && !netstandard). And then the two (corefx csproj condition vs the conditional compilation in this repo) appear to conflict.

I am not able to understand how this #if compares to the conditional compilation in the CoreFx csproj. Hence please help with what I should actually use.

Copy link
Member

Choose a reason for hiding this comment

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

I am thoroughly confused when it comes to nomenclature such as netstandard vs netfx, netcoreapp, etc.

netcoreapp means .NET Core.
netfx means .NET Framework.

Both the .NET Core and .NET Framework are concrete implementations of a runtime that executes .NET code. This means you can actually run code on them. The .NET Framework ships as part of Windows and only runs on Windows. .NET Core runs on Windows, Linux and OSX.

You can think of netstandard as an abstraction a developer can target that will allow their code to run on any concrete runtime: .NET Core, .NET Framework, Mono, etc. netstandard isn't a runtime itself, but it is a contract that the concrete runtimes implement.

So do you mean to say that I should put the using statement and entire new constructor under #if (!netfx && !netstandard).

I think it should actually be under #if CORECLR (which is the DefineConstant used by coreclr to mean "this is netcoreapp").

When building this code in dotnet/corefx for netfx and netstandard that DefineConstant won't be defined, so this code won't be compiled.

Copy link
Author

Choose a reason for hiding this comment

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

When building this code in dotnet/corefx for netfx and netstandard that DefineConstant won't be defined, so this code won't be compiled.

Thanks @eerhardt for the explanation. Actually what confused is me that if netstandard is the standard and netfx is implementation then why was I supposed to use both in that #if. And just when I thought I had reconfirmed my understanding by reading your above reply, I again got stuck at the last line in which you have mentioned netfx and netstandard in same line :-) . I thought netstandard includes netfx (standard and implementation relationship). So did you want to mention netcoreapp instead of netstandard ...? In other words is it necessary to explicitly say netfx when I say netstandard... Hope I am not creating more confusion.

As for the PR, I am replacing it iwth CORECLR as you have mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

iwth CORECLR as you have mentioned.

That will not compile on CoreRT and ProjectN. It would need to be more combined with more conditions there. I think #if netcoreapp and defining netcoreapp when building S.P.CoreLib as you have proposed originally is probably the best choice.

Copy link
Author

Choose a reason for hiding this comment

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

@jkotas Thank you. I have pushed a commit that defines netcoreapp and uses in the .cs file.

Copy link
Member

Choose a reason for hiding this comment

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

nit: put this after all the System.* using directives

Copy link
Author

Choose a reason for hiding this comment

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

I thought we sort them alphabetically...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but for ones other than System (like internal), we group them separately (with system first). At least that is what I have seen (and followed) elsewhere.

So, something like:

using System.X;
using System.Y;
using System.Z;

using Internal.A;
using Internal.B;
using Internal.C;

Copy link
Author

Choose a reason for hiding this comment

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

Between, this PR is already merged and closed. So perhaps we can do all this in the optimization issue along with other clean up?

Copy link
Member

Choose a reason for hiding this comment

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

Of course. That would be great.

#endif
using System.Globalization;
@@ -768,6 +768,7 @@ private Vector(ref Register existingRegister)
this.register = existingRegister;
}

#if netcoreapp
/// <summary>
/// Constructs a vector from the given span. The span must contain at least Vector'T.Count elements.
Copy link
Member

@ahsonkhan ahsonkhan Mar 8, 2018

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Yup. But I kept it same like other instances in the file for consistency. E.g. here i.e. below code,

        /// Constructs a vector from the given array. The size of the given array must be at least Vector'T.Count.
        /// </summary>
        [Intrinsic]
        public unsafe Vector(T[] values) : this(values, 0) { }

Do we want to fix this as part of the optimization issue #16745?

Copy link
Member

Choose a reason for hiding this comment

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

This can be addressed separately and we can make the changes to the whole file at the same time. Good point about consistency with other places in the file.

/// </summary>
@@ -796,6 +797,7 @@ public Vector(Span<T> values)
throw new NotSupportedException(SR.Arg_TypeNotSupported);
}
}
#endif
#endregion Constructors

#region Public Instance Methods
4 changes: 3 additions & 1 deletion src/mscorlib/shared/System/Numerics/Vector.tt
Original file line number Diff line number Diff line change
@@ -7,7 +7,7 @@
<#@ import namespace="System.Runtime.InteropServices" #>
<#@ include file="GenerationConfig.ttinclude" #><# GenerateCopyrightHeader(); #>

#if (!netfx && !netstandard)
#if netcoreapp
using Internal.Runtime.CompilerServices;
#endif
using System.Globalization;
@@ -288,6 +288,7 @@ namespace System.Numerics
this.register = existingRegister;
}

#if netcoreapp
/// <summary>
/// Constructs a vector from the given span. The span must contain at least Vector'T.Count elements.
/// </summary>
@@ -307,6 +308,7 @@ namespace System.Numerics
throw new NotSupportedException(SR.Arg_TypeNotSupported);
}
}
#endif
#endregion Constructors

#region Public Instance Methods