From 436ecf9af09d745c65f4fe04ee4963153a030b53 Mon Sep 17 00:00:00 2001 From: Jean Hominal Date: Sun, 17 Jul 2022 18:00:15 +0200 Subject: [PATCH 1/2] reorder methods in IFuture --- Squared/Threading/Future.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Squared/Threading/Future.cs b/Squared/Threading/Future.cs index 7cb44dbc8..87d9daf7c 100644 --- a/Squared/Threading/Future.cs +++ b/Squared/Threading/Future.cs @@ -154,14 +154,14 @@ Type ResultType { void RegisterHandlers (OnFutureResolved completeHandler, OnFutureResolved disposeHandler); void RegisterHandlers (OnFutureResolvedWithData completeHandler, OnFutureResolvedWithData disposeHandler, object userData); void RegisterOnResolved (Action handler); + void RegisterOnResolved (OnFutureResolved handler); void RegisterOnResolved (OnFutureResolvedWithData handler, object userData); void RegisterOnComplete (Action handler); + void RegisterOnComplete (OnFutureResolved handler); void RegisterOnComplete (OnFutureResolvedWithData handler, object userData); void RegisterOnDispose (Action handler); - void RegisterOnDispose (OnFutureResolvedWithData handler, object userData); - void RegisterOnResolved (OnFutureResolved handler); - void RegisterOnComplete (OnFutureResolved handler); void RegisterOnDispose (OnFutureResolved handler); + void RegisterOnDispose (OnFutureResolvedWithData handler, object userData); void RegisterOnErrorCheck (Action handler); bool CopyFrom (IFuture source); } From dfa058e39c85b7e112bebf4e9b81e4ed32036dac Mon Sep 17 00:00:00 2001 From: Jean Hominal Date: Sun, 17 Jul 2022 21:31:36 +0200 Subject: [PATCH 2/2] make first review comments on Future.cs --- Squared/Threading/Future.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Squared/Threading/Future.cs b/Squared/Threading/Future.cs index 87d9daf7c..438a7bde5 100644 --- a/Squared/Threading/Future.cs +++ b/Squared/Threading/Future.cs @@ -64,6 +64,7 @@ public FutureHandlerException (IFuture future, Delegate handler, string message) } internal static class FutureHelpers { + // REVIEW: Pretty sure I would like to rewrite this RunInThreadThunk subsystem but not 100% sure how public static readonly WaitCallback RunInThreadHelper; static FutureHelpers () { @@ -75,6 +76,7 @@ internal static void _RunInThreadHelper (object state) { try { thunk.Invoke(); } catch (System.Reflection.TargetInvocationException ex) { + // REVIEW: Which cases is this catch block for? thunk.Fail(ex.InnerException); } catch (Exception ex) { thunk.Fail(ex); @@ -190,6 +192,8 @@ public SignalFuture (bool signaled) public static class Future { public enum State : int { + // REVIEW: I guess Unknown and Disposing are removed because + // you did not want these states to be part of the public API? Empty = 0, // Unknown = 1, CompletedWithValue = 2, @@ -254,6 +258,10 @@ private sealed class WaitForFirstThunk { public WaitForFirstThunk (IEnumerable futures) { OnFutureResolved handler = null; + // REVIEW: On a theoretical level, I am worried that, if the enumerable is + // empty and not detected by the condition below, or if all futures are null + // then Composite never completes. + // On a practical level, I suspect that it has not happened often. ;) if ( ((futures as System.Collections.IList)?.Count == 0) || ((futures as Array)?.Length == 0) @@ -661,6 +669,10 @@ public void RegisterHandlers (Action onComplete, Action onDispose) { } // FIXME: Set state to indeterminate once instead of twice + // REVIEW: I think that it would be possible to set state to indeterminate only once + // by splitting RegisterHandler_Impl in two: + // * a Disposable struct that would call TrySetIndeterminate on instantiation, and ClearIndeterminate on Dispose + // * a function that does the work of registering or running the handler if (!RegisterHandler_Impl(onComplete, HandlerType.Completed)) RegisterHandler_Impl(onDispose, HandlerType.Disposed); }