diff --git a/Squared/Threading/Future.cs b/Squared/Threading/Future.cs index 7cb44dbc8..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); @@ -154,14 +156,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); } @@ -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); }