Move SynchronizationContext to shared partition #22389
Conversation
marek-safar
force-pushed
the
shared4
branch
2 times, most recently
from
b99bd5f to
87831b1
Compare
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| #if CORERT | ||
| using Thread = Internal.Runtime.Augments.RuntimeThread; |
There was a problem hiding this comment.
The #if CORERT should not be necessary. Thread is already in the same namespace and it will be resolved to the correct class. The using will just become no-op on CoreCLR.
There was a problem hiding this comment.
This is for CoreRT which still does not have Thread (AFAIK)
There was a problem hiding this comment.
I meant to keep the using and drop the #if around it. Sorry, I wasn't clear about it :)
There was a problem hiding this comment.
I see. I have removed it although there is a mixture of both used across the files
There was a problem hiding this comment.
It has to be there if the main class is not in the System.Threading namespace (eg. AsyncMethodBuilders). I agree it's confusing and I will be happy to address that once the strategy is decided upon (#22032).
|
Btw, the Azure Pipelines CI can usually be restarted by "/AzurePipelines run coreclr-ci" comment. Unfortunately it's been broken for about a week now (dotnet/corefx#34747 (comment)) and it still didn't work as of few minutes ago :-/ |
| { | ||
| } | ||
|
|
||
| #if !FEATURE_APPX |
There was a problem hiding this comment.
This should be #if !FEATURE_APPX && !ENABLE_WINRT to make it work for ProjectN.
| <Compile Include="$(BclSourcesRoot)\System\Globalization\GlobalizationMode.Windows.cs" /> | ||
| <Compile Include="$(BclSourcesRoot)\System\Threading\ClrThreadPoolBoundHandle.Windows.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(FeatureAppX)' == 'true' or '$(EnableWinRT)' == 'true'"> |
There was a problem hiding this comment.
EnableWinRT is not ever defined in CoreCLR. This can be just '$(FeatureAppX)' == 'true'.
| throw new ArgumentNullException(nameof(waitHandles)); | ||
| } | ||
|
|
||
| return WaitHelperNative(waitHandles, waitAll, millisecondsTimeout); |
There was a problem hiding this comment.
This method logically belongs to WaitHandle. Forwarding this onto a method on WaitHandle like it is done in CoreRT seems better to me.
There was a problem hiding this comment.
(Maybe even keep the null check for the argument in the WaitHandle part.)
|
LGTM otherwise. Thanks! |
* Move SynchronizationContext to shared partition * Move WaitHelperNative to WaitHandle Signed-off-by: dotnet-bot <[email protected]>
* Move SynchronizationContext to shared partition * Move WaitHelperNative to WaitHandle Signed-off-by: dotnet-bot <[email protected]>
* Move SynchronizationContext to shared partition * Move WaitHelperNative to WaitHandle Signed-off-by: dotnet-bot <[email protected]>
* Move SynchronizationContext to shared partition * Move WaitHelperNative to WaitHandle Signed-off-by: dotnet-bot <[email protected]>
* Move SynchronizationContext to shared partition * Move WaitHelperNative to WaitHandle Signed-off-by: dotnet-bot <[email protected]>
* Move SynchronizationContext to shared partition * Move WaitHelperNative to WaitHandle Signed-off-by: dotnet-bot <[email protected]>
* Move SynchronizationContext to shared partition * Move WaitHelperNative to WaitHandle Commit migrated from dotnet/coreclr@382dbe4
3 participants
No description provided.