-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean-up project files and MSBuild logic #3893
Changes from all commits
13bfbf2
e606ae8
c6d9fe4
06362b2
7e37935
8d205a1
7586eeb
53a7635
54ffaf9
7c61d09
aa5f174
a52ad35
c891e29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,17 +24,19 @@ | |
</Description> | ||
Nirmal4G marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<PackageTags>Windows;Community;Toolkit;WCT;UWP;core;standard;unsafe;span;memory;string;array;stream;buffer;extensions;helpers;parallel;performance</PackageTags> | ||
</PropertyGroup> | ||
|
||
<Choose> | ||
<When Condition=" '$(TargetFramework)' == 'netstandard1.4' "> | ||
<When Condition="'$(TargetFramework)' == 'netstandard1.4'"> | ||
<!-- | ||
.NET Standard 1.4 lacks the [Pure] attribute, the Rectangle primitive, | ||
the Span<T> and Memory<T> types, the Vector<T> primitive and related APIs, | ||
ValueTask and ValueTask<T>, the Parallel class and the Unsafe class. | ||
We also need to reference the System.Runtime.CompilerServices.Unsafe package directly, | ||
even though System.Memory references it already, as we need a more recent version than | ||
the one bundled with it. This is so that we can use the Unsafe.Unbox<T> method, | ||
which is used by the Box<T> type in the package. | ||
--> | ||
Comment on lines
+29
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with the new blank lines around the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a comment is common to a whole block of code and not just the following line, placing one level above gives clarity. Same remarks as above for the rest of the changes. |
||
<ItemGroup> | ||
|
||
<!-- .NET Standard 1.4 lacks the [Pure] attribute, the Rectangle primitive, | ||
the Span<T> and Memory<T> types, the Vector<T> primitive and related APIs, | ||
ValueTask and ValueTask<T>, the Parallel class and the Unsafe class. | ||
We also need to reference the System.Runtime.CompilerServices.Unsafe package directly, | ||
even though System.Memory references it already, as we need a more recent version than | ||
the one bundled with it. This is so that we can use the Unsafe.Unbox<T> method, | ||
which is used by the Box<T> type in the package. --> | ||
<PackageReference Include="System.Diagnostics.Contracts" Version="4.3.0" /> | ||
<PackageReference Include="System.Drawing.Primitives" Version="4.3.0" /> | ||
<PackageReference Include="System.Memory" Version="4.5.4" /> | ||
|
@@ -44,65 +46,70 @@ | |
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="5.0.0" /> | ||
</ItemGroup> | ||
</When> | ||
<When Condition=" '$(TargetFramework)' == 'netstandard2.0' "> | ||
<ItemGroup> | ||
|
||
<!-- .NET Standard 2.0 doesn't have the Span<T>, HashCode and ValueTask types --> | ||
<When Condition="'$(TargetFramework)' == 'netstandard2.0'"> | ||
<!-- .NET Standard 2.0 doesn't have the Span<T>, HashCode and ValueTask types --> | ||
<ItemGroup> | ||
<PackageReference Include="Microsoft.Bcl.HashCode" Version="1.1.0" /> | ||
<PackageReference Include="System.Memory" Version="4.5.4" /> | ||
<PackageReference Include="System.Threading.Tasks.Extensions" Version="4.5.4" /> | ||
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="5.0.0" /> | ||
</ItemGroup> | ||
</When> | ||
<When Condition=" '$(TargetFramework)' == 'netstandard2.1' "> | ||
<ItemGroup> | ||
|
||
<!-- .NET Standard 2.1 doesn't have the Unsafe type --> | ||
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="5.0.0" /> | ||
</ItemGroup> | ||
<PropertyGroup> | ||
|
||
<!-- NETSTANDARD2_1_OR_GREATER: includes both .NET Standard 2.1 and .NET Core 3.1. | ||
This is needed because .NET Core 3.1 will be a separate package than .NET Standard 2.1. --> | ||
<When Condition="'$(TargetFramework)' == 'netstandard2.1'"> | ||
<!-- | ||
NETSTANDARD2_1_OR_GREATER: includes both .NET Standard 2.1 and .NET Core 3.1. | ||
This is needed because .NET Core 3.1 will be a separate package than .NET Standard 2.1. | ||
<!-- SPAN_RUNTIME_SUPPORT: define a constant to indicate runtimes with runtime support for | ||
the fast Span<T> type, as well as some overloads with Span<T> parameters (eg. Stream.Write). | ||
In particular, these are runtimes which are able to create Span<T> instances from just | ||
a managed reference, which can be used to slice arbitrary objects not technically supported. | ||
This API (MemoryMarshal.CreateSpan) is not part of .NET Standard 2.0, but it is still | ||
available on .NET Core 2.1. So by using this constant, we can make sure to expose those | ||
APIs relying on that method on all target frameworks that are able to support them. --> | ||
<DefineConstants>NETSTANDARD2_1_OR_GREATER;SPAN_RUNTIME_SUPPORT</DefineConstants> | ||
</PropertyGroup> | ||
</When> | ||
<When Condition=" '$(TargetFramework)' == 'net5.0' "> | ||
SPAN_RUNTIME_SUPPORT: define a constant to indicate runtimes with runtime support for | ||
the fast Span<T> type, as well as some overloads with Span<T> parameters (eg. Stream.Write). | ||
In particular, these are runtimes which are able to create Span<T> instances from just | ||
a managed reference, which can be used to slice arbitrary objects not technically supported. | ||
This API (MemoryMarshal.CreateSpan) is not part of .NET Standard 2.0, but it is still | ||
available on .NET Core 2.1. So by using this constant, we can make sure to expose those | ||
APIs relying on that method on all target frameworks that are able to support them. | ||
--> | ||
<PropertyGroup> | ||
<DefineConstants>NETSTANDARD2_1_OR_GREATER;SPAN_RUNTIME_SUPPORT</DefineConstants> | ||
</PropertyGroup> | ||
</When> | ||
<When Condition=" '$(TargetFramework)' == 'netcoreapp3.1' "> | ||
<!-- .NET Standard 2.1 doesn't have the Unsafe type --> | ||
<ItemGroup> | ||
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="5.0.0" /> | ||
</ItemGroup> | ||
</When> | ||
|
||
<When Condition="'$(TargetFramework)' == 'net5.0'"> | ||
<PropertyGroup> | ||
<DefineConstants>NETSTANDARD2_1_OR_GREATER;SPAN_RUNTIME_SUPPORT</DefineConstants> | ||
</PropertyGroup> | ||
</When> | ||
|
||
<!-- NETCORE_RUNTIME: to avoid issues with APIs that assume a specific memory layout, we define a | ||
.NET Core runtime constant to indicate either .NET Core 2.1 or .NET Core 3.1. These are | ||
runtimes with the same overall memory layout for objects (in particular: strings, SZ arrays, | ||
and ND arrays). We can use this constant to make sure that APIs that are exclusively available | ||
for .NET Standard targets do not make any assumtpion of any internals of the runtime being | ||
actually used by the consumers. .NET 5.0 would fall into this category as well, but we don't | ||
need to include that target as it offers APIs that don't require runtime-based workarounds.--> | ||
<When Condition="'$(TargetFramework)' == 'netcoreapp3.1'"> | ||
<!-- | ||
NETCORE_RUNTIME: to avoid issues with APIs that assume a specific memory layout, we define a | ||
.NET Core runtime constant to indicate either .NET Core 2.1 or .NET Core 3.1. These are | ||
runtimes with the same overall memory layout for objects (in particular: strings, SZ arrays, | ||
and ND arrays). We can use this constant to make sure that APIs that are exclusively available | ||
for .NET Standard targets do not make any assumtpion of any internals of the runtime being | ||
actually used by the consumers. .NET 5.0 would fall into this category as well, but we don't | ||
need to include that target as it offers APIs that don't require runtime-based workarounds. | ||
--> | ||
<PropertyGroup> | ||
<DefineConstants>NETSTANDARD2_1_OR_GREATER;SPAN_RUNTIME_SUPPORT;NETCORE_RUNTIME</DefineConstants> | ||
</PropertyGroup> | ||
</When> | ||
<When Condition=" '$(TargetFramework)' == 'netcoreapp2.1' "> | ||
<ItemGroup> | ||
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="5.0.0" /> | ||
</ItemGroup> | ||
</When> | ||
|
||
<When Condition="'$(TargetFramework)' == 'netcoreapp2.1'"> | ||
<PropertyGroup> | ||
<DefineConstants>SPAN_RUNTIME_SUPPORT;NETCORE_RUNTIME</DefineConstants> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="5.0.0" /> | ||
</ItemGroup> | ||
</When> | ||
</Choose> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,5 @@ | ||
<!-- | ||
This file contains Runtime Directives, specifications about types your application accesses | ||
through reflection and other dynamic code patterns. Runtime Directives are used to control the | ||
.NET Native optimizer and ensure that it does not remove code accessed by your library. If your | ||
library does not do any reflection, then you generally do not need to edit this file. However, | ||
if your library reflects over types, especially types passed to it or derived from its types, | ||
then you should write Runtime Directives. | ||
The most common use of reflection in libraries is to discover information about types passed | ||
to the library. Runtime Directives have three ways to express requirements on types passed to | ||
your library. | ||
1. Parameter, GenericParameter, TypeParameter, TypeEnumerableParameter | ||
Use these directives to reflect over types passed as a parameter. | ||
2. SubTypes | ||
Use a SubTypes directive to reflect over types derived from another type. | ||
3. AttributeImplies | ||
Use an AttributeImplies directive to indicate that your library needs to reflect over | ||
types or methods decorated with an attribute. | ||
For more information on writing Runtime Directives for libraries, please visit | ||
https://go.microsoft.com/fwlink/?LinkID=391919 | ||
--> | ||
<Directives xmlns="http://schemas.microsoft.com/netfx/2013/01/metadata"> | ||
<Library Name="Microsoft.Toolkit.Uwp.Connectivity"> | ||
|
||
<!-- add directives for your library here --> | ||
|
||
<!-- add directives for your library here --> | ||
</Library> | ||
</Directives> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big fan of this change - all XML comments had a blank line before them for consistency (and to more easily distinguish them from normal blocks while just glancing at code), whereas this change breaks that 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree on the same but only when the lines look congested. If they are easy on the eyes (in this case) then removing the un-necessary blank lines wouldn't affect readability.
This is also why I put multi-line comment's start and end tag on separate lines to avoid having to add additional new lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm either way on the extra blank line, but especially with the ones below, the comments are about the packages, so it seems a bit odd to move them to outside the item group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is about all 3 packages which are in the
ItemGroup
, not justSystem.Diagnostics.Contracts
! That's why I moved them.