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

CoreFX #24343 Vector Ctor using Span #16733

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1912fe1
CoreFX #24343 Vector using Span
WinCPP Mar 1, 2018
52e95ad
CoreFX #24343 Vector using Span
WinCPP Mar 1, 2018
71d5215
CoreFX #24343 Vector using Span
WinCPP Mar 1, 2018
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
1 change: 1 addition & 0 deletions src/mscorlib/System.Private.CoreLib.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
4 changes: 3 additions & 1 deletion src/mscorlib/shared/System/Numerics/Vector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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;
Expand Down Expand Up @@ -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 •
edited
Loading

Choose a reason for hiding this comment

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

nit: use proper xml formatting (see
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Buffers/OwnedMemory.cs#L16 / https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Span.cs#L90).

The span must contain at least Vector<typeparamref name="T"/>.Count elements.

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>
Expand Down Expand Up @@ -796,6 +797,7 @@ public Vector(Span<T> values)
throw new NotSupportedException(SR.Arg_TypeNotSupported);
}
}
#endif
#endregion Constructors

#region Public Instance Methods
Expand Down
4 changes: 3 additions & 1 deletion src/mscorlib/shared/System/Numerics/Vector.tt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>
Expand All @@ -307,6 +308,7 @@ namespace System.Numerics
throw new NotSupportedException(SR.Arg_TypeNotSupported);
}
}
#endif
#endregion Constructors

#region Public Instance Methods
Expand Down