Turn TryAbortResult into enum class

It's too easy to implicitly cast it to bool, as we did in several tests.
Also, move TryAbortResult out of CancelableTaskManager to avoid too much
typing when referencing one of the enum values.

R=mstarzinger@chromium.org

Bug: v8:8238
Change-Id: Ia3fa8597428876217bc86f9b8b31c21ae4846fa1
Reviewed-on: https://chromium-review.googlesource.com/c/1326027
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57363}

src/cancelable-task.cc

@@ -47,8 +47,7 @@ void CancelableTaskManager::RemoveFinishedTask(CancelableTaskManager::Id id) {
   cancelable_tasks_barrier_.NotifyOne();
 }
 
-CancelableTaskManager::TryAbortResult CancelableTaskManager::TryAbort(
-    CancelableTaskManager::Id id) {
+TryAbortResult CancelableTaskManager::TryAbort(CancelableTaskManager::Id id) {
   base::MutexGuard guard(&mutex_);
   auto entry = cancelable_tasks_.find(id);
   if (entry != cancelable_tasks_.end()) {
@@ -57,12 +56,12 @@ CancelableTaskManager::TryAbortResult CancelableTaskManager::TryAbort(
       // Cannot call RemoveFinishedTask here because of recursive locking.
       cancelable_tasks_.erase(entry);
       cancelable_tasks_barrier_.NotifyOne();
-      return kTaskAborted;
+      return TryAbortResult::kTaskAborted;
     } else {
-      return kTaskRunning;
+      return TryAbortResult::kTaskRunning;
     }
   }
-  return kTaskRemoved;
+  return TryAbortResult::kTaskRemoved;
 }
 
 void CancelableTaskManager::CancelAndWait() {
@@ -91,12 +90,12 @@ void CancelableTaskManager::CancelAndWait() {
   }
 }
 
-CancelableTaskManager::TryAbortResult CancelableTaskManager::TryAbortAll() {
+TryAbortResult CancelableTaskManager::TryAbortAll() {
   // Clean up all cancelable fore- and background tasks. Tasks are canceled on
   // the way if possible, i.e., if they have not started yet.
   base::MutexGuard guard(&mutex_);
 
-  if (cancelable_tasks_.empty()) return kTaskRemoved;
+  if (cancelable_tasks_.empty()) return TryAbortResult::kTaskRemoved;
 
   for (auto it = cancelable_tasks_.begin(); it != cancelable_tasks_.end();) {
     if (it->second->Cancel()) {
@@ -106,7 +105,8 @@ CancelableTaskManager::TryAbortResult CancelableTaskManager::TryAbortAll() {
     }
   }
 
-  return cancelable_tasks_.empty() ? kTaskAborted : kTaskRunning;
+  return cancelable_tasks_.empty() ? TryAbortResult::kTaskAborted
+                                   : TryAbortResult::kTaskRunning;
 }
 
 CancelableTask::CancelableTask(Isolate* isolate)

src/cancelable-task.h

@@ -19,6 +19,13 @@ namespace internal {
 class Cancelable;
 class Isolate;
 
+// The possible outcomes of trying to abort a job are:
+// (1) The task is already finished running or was canceled before and
+//     thus has been removed from the manager.
+// (2) The task is currently running and cannot be canceled anymore.
+// (3) The task is not yet running (or finished) so it is canceled and
+//     removed.
+enum class TryAbortResult { kTaskRemoved, kTaskRunning, kTaskAborted };
 
 // Keeps track of cancelable tasks. It is possible to register and remove tasks
 // from any fore- and background task/thread.
@@ -33,14 +40,7 @@ class V8_EXPORT_PRIVATE CancelableTaskManager {
   // Must not be called after CancelAndWait.
   Id Register(Cancelable* task);
 
-  // Try to abort running a task identified by {id}. The possible outcomes are:
-  // (1) The task is already finished running or was canceled before and
-  //     thus has been removed from the manager.
-  // (2) The task is currently running and cannot be canceled anymore.
-  // (3) The task is not yet running (or finished) so it is canceled and
-  //     removed.
-  //
-  enum TryAbortResult { kTaskRemoved, kTaskRunning, kTaskAborted };
+  // Try to abort running a task identified by {id}.
   TryAbortResult TryAbort(Id id);
 
   // Tries to cancel all remaining registered tasks. The return value indicates

src/heap/concurrent-marking.cc

@@ -743,7 +743,7 @@ bool ConcurrentMarking::Stop(StopRequest stop_request) {
     for (int i = 1; i <= task_count_; i++) {
       if (is_pending_[i]) {
         if (task_manager->TryAbort(cancelable_id_[i]) ==
-            CancelableTaskManager::kTaskAborted) {
+            TryAbortResult::kTaskAborted) {
           is_pending_[i] = false;
           --pending_task_count_;
         } else if (stop_request == StopRequest::PREEMPT_TASKS) {

src/heap/item-parallel-job.cc

@@ -119,7 +119,7 @@ void ItemParallelJob::Run(const std::shared_ptr<Counters>& async_counters) {
   // Wait for background tasks.
   for (size_t i = 0; i < num_tasks; i++) {
     if (cancelable_task_manager_->TryAbort(task_ids[i]) !=
-        CancelableTaskManager::kTaskAborted) {
+        TryAbortResult::kTaskAborted) {
       pending_tasks_->Wait();
     }
   }

src/heap/spaces.cc

@@ -291,7 +291,7 @@ void MemoryAllocator::Unmapper::FreeQueuedChunks() {
 void MemoryAllocator::Unmapper::CancelAndWaitForPendingTasks() {
   for (int i = 0; i < pending_unmapping_tasks_; i++) {
     if (heap_->isolate()->cancelable_task_manager()->TryAbort(task_ids_[i]) !=
-        CancelableTaskManager::kTaskAborted) {
+        TryAbortResult::kTaskAborted) {
       pending_unmapping_tasks_semaphore_.Wait();
     }
   }

src/heap/sweeper.cc

@@ -211,7 +211,7 @@ void Sweeper::AbortAndWaitForTasks() {
 
   for (int i = 0; i < num_tasks_; i++) {
     if (heap_->isolate()->cancelable_task_manager()->TryAbort(task_ids_[i]) !=
-        CancelableTaskManager::kTaskAborted) {
+        TryAbortResult::kTaskAborted) {
       pending_sweeper_tasks_semaphore_.Wait();
     } else {
       // Aborted case.
@@ -524,7 +524,7 @@ void Sweeper::EnsureIterabilityCompleted() {
 
   if (FLAG_concurrent_sweeping && iterability_task_started_) {
     if (heap_->isolate()->cancelable_task_manager()->TryAbort(
-            iterability_task_id_) != CancelableTaskManager::kTaskAborted) {
+            iterability_task_id_) != TryAbortResult::kTaskAborted) {
       iterability_task_semaphore_.Wait();
     }
     iterability_task_started_ = false;

test/unittests/cancelable-tasks-unittest.cc

@@ -128,8 +128,8 @@ TEST(CancelableTask, SequentialMultipleTasks) {
   EXPECT_EQ(GetValue(&result2), 2);
 
   manager.CancelAndWait();
-  EXPECT_FALSE(manager.TryAbort(1));
-  EXPECT_FALSE(manager.TryAbort(2));
+  EXPECT_EQ(TryAbortResult::kTaskRemoved, manager.TryAbort(1));
+  EXPECT_EQ(TryAbortResult::kTaskRemoved, manager.TryAbort(2));
 }
 
 
@@ -182,7 +182,7 @@ TEST(CancelableTask, RemoveBeforeCancelAndWait) {
   ThreadedRunner runner1(task1);
   CancelableTaskManager::Id id = task1->id();
   EXPECT_EQ(id, 1u);
-  EXPECT_TRUE(manager.TryAbort(id));
+  EXPECT_EQ(TryAbortResult::kTaskAborted, manager.TryAbort(id));
   runner1.Start();
   runner1.Join();
   manager.CancelAndWait();
@@ -200,23 +200,23 @@ TEST(CancelableTask, RemoveAfterCancelAndWait) {
   runner1.Start();
   runner1.Join();
   manager.CancelAndWait();
-  EXPECT_FALSE(manager.TryAbort(id));
+  EXPECT_EQ(TryAbortResult::kTaskRemoved, manager.TryAbort(id));
   EXPECT_EQ(GetValue(&result1), 1);
 }
 
 
 TEST(CancelableTask, RemoveUnmanagedId) {
   CancelableTaskManager manager;
-  EXPECT_FALSE(manager.TryAbort(1));
-  EXPECT_FALSE(manager.TryAbort(2));
+  EXPECT_EQ(TryAbortResult::kTaskRemoved, manager.TryAbort(1));
+  EXPECT_EQ(TryAbortResult::kTaskRemoved, manager.TryAbort(2));
   manager.CancelAndWait();
-  EXPECT_FALSE(manager.TryAbort(1));
-  EXPECT_FALSE(manager.TryAbort(3));
+  EXPECT_EQ(TryAbortResult::kTaskRemoved, manager.TryAbort(1));
+  EXPECT_EQ(TryAbortResult::kTaskRemoved, manager.TryAbort(3));
 }
 
 TEST(CancelableTask, EmptyTryAbortAll) {
   CancelableTaskManager manager;
-  EXPECT_EQ(manager.TryAbortAll(), CancelableTaskManager::kTaskRemoved);
+  EXPECT_EQ(TryAbortResult::kTaskRemoved, manager.TryAbortAll());
 }
 
 TEST(CancelableTask, ThreadedMultipleTasksNotRunTryAbortAll) {
@@ -227,7 +227,7 @@ TEST(CancelableTask, ThreadedMultipleTasksNotRunTryAbortAll) {
   TestTask* task2 = new TestTask(&manager, &result2, TestTask::kCheckNotRun);
   ThreadedRunner runner1(task1);
   ThreadedRunner runner2(task2);
-  EXPECT_EQ(manager.TryAbortAll(), CancelableTaskManager::kTaskAborted);
+  EXPECT_EQ(TryAbortResult::kTaskAborted, manager.TryAbortAll());
   // Tasks are canceled, hence the runner will bail out and not update result.
   runner1.Start();
   runner2.Start();
@@ -251,7 +251,7 @@ TEST(CancelableTask, ThreadedMultipleTasksStartedTryAbortAll) {
   // Busy wait on result to make sure task1 is done.
   while (GetValue(&result1) == 0) {
   }
-  EXPECT_EQ(manager.TryAbortAll(), CancelableTaskManager::kTaskRunning);
+  EXPECT_EQ(TryAbortResult::kTaskRunning, manager.TryAbortAll());
   runner2.Start();
   runner1.Join();
   runner2.Join();