changed api of getNextPath, not modifying builder but returning PathR…

…esult instead - will lead to more flexibility handling the result
arangodb · mchacki · Jul 7, 2021 · Apr 20, 2021 · Apr 22, 2021 · Apr 22, 2021
commit cb9f78f0e683f12c1036682431625ab35c17471b
diff --git a/arangod/Graph/Enumerators/OneSidedEnumerator.cpp b/arangod/Graph/Enumerators/OneSidedEnumerator.cpp
@@ -55,8 +55,7 @@ OneSidedEnumerator<QueueType, PathStoreType, ProviderType, PathValidator>::OneSi
       _queue(resourceMonitor),
       _provider(std::move(forwardProvider)),
       _validator(_interior),
-      _interior(resourceMonitor),
-      _resultPath{_provider} {}
+      _interior(resourceMonitor) {}
 
 template <class QueueType, class PathStoreType, class ProviderType, class PathValidator>
 OneSidedEnumerator<QueueType, PathStoreType, ProviderType, PathValidator>::~OneSidedEnumerator() {
@@ -153,7 +152,8 @@ void OneSidedEnumerator<QueueType, PathStoreType, ProviderType, PathValidator>::
  * @return false No path found, result has not been changed.
  */
 template <class QueueType, class PathStoreType, class ProviderType, class PathValidator>
-bool OneSidedEnumerator<QueueType, PathStoreType, ProviderType, PathValidator>::getNextPath(VPackBuilder& result) {
+auto OneSidedEnumerator<QueueType, PathStoreType, ProviderType, PathValidator>::getNextPath()
+    -> std::optional<SingleProviderPathResult<ProviderType, typename ProviderType::Step>> {
   while (!isDone()) {
     searchMoreResults();
 
@@ -165,15 +165,15 @@ bool OneSidedEnumerator<QueueType, PathStoreType, ProviderType, PathValidator>::
       // everything in to the _resultPath object
       // and then iterate again to return the path
       // we should be able to return the path in the first go.
-      _resultPath.clear();
-      _interior.buildPath(vertex, _resultPath);
-      TRI_ASSERT(!_resultPath.isEmpty());
+      auto resultPath = SingleProviderPathResult<ProviderType, Step>{_provider};
+      _interior.buildPath(vertex, resultPath);
+      TRI_ASSERT(!resultPath.isEmpty());
       _results.pop_back();
-      _resultPath.toVelocyPack(result);
-      return true;
+
+      return resultPath; // TODO: Please let us check alternatives here (e.g. std::move)
     }
   }
-  return false;
+  return std::nullopt;
 }
 
 template <class QueueType, class PathStoreType, class ProviderType, class PathValidator>

diff --git a/arangod/Graph/Enumerators/OneSidedEnumerator.h b/arangod/Graph/Enumerators/OneSidedEnumerator.h
@@ -49,9 +49,6 @@ namespace graph {
 
 struct OneSidedEnumeratorOptions;
 
-template <class ProviderType, class Step>
-class PathResult;
-
 template <class QueueType, class PathStoreType, class ProviderType, class PathValidatorType>
 class OneSidedEnumerator {
  public:
@@ -106,7 +103,7 @@ class OneSidedEnumerator {
    * @return true Found and written a path, result is modified.
    * @return false No path found, result has not been changed.
    */
-  bool getNextPath(arangodb::velocypack::Builder& result);
+  auto getNextPath() -> std::optional<SingleProviderPathResult<ProviderType, Step>>;
 
   /**
    * @brief Skip the next Path, like getNextPath, but does not return the path.
@@ -151,8 +148,6 @@ class OneSidedEnumerator {
 
   // This stores all paths processed
   PathStoreType _interior;
-
-  SingleProviderPathResult<ProviderType, Step> _resultPath;
 };
 }  // namespace graph
 }  // namespace arangodb
diff --git a/tests/Graph/DFSFinderTest.cpp b/tests/Graph/DFSFinderTest.cpp
@@ -248,8 +248,9 @@ TEST_P(DFSFinderTest, no_path_exists) {
   EXPECT_FALSE(finder.isDone());
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
     pathEquals(result.slice(), {91});
     pathStructureValid(result.slice(), 0);
     EXPECT_TRUE(finder.isDone());
@@ -258,7 +259,7 @@ TEST_P(DFSFinderTest, no_path_exists) {
   {
     result.clear();
     // Try again to make sure we stay at non-existing
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_FALSE(hasPath);
     EXPECT_TRUE(result.isEmpty());
     EXPECT_TRUE(finder.isDone());
@@ -282,8 +283,10 @@ TEST_P(DFSFinderTest, path_depth_0) {
   EXPECT_FALSE(finder.isDone());
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathEquals(result.slice(), {1});
     pathStructureValid(result.slice(), 0);
     EXPECT_TRUE(finder.isDone());
@@ -292,7 +295,7 @@ TEST_P(DFSFinderTest, path_depth_0) {
   {
     result.clear();
     // Try again to make sure we stay at non-existing
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_FALSE(hasPath);
     EXPECT_TRUE(result.isEmpty());
     EXPECT_TRUE(finder.isDone());
@@ -315,8 +318,10 @@ TEST_P(DFSFinderTest, path_depth_1) {
   EXPECT_FALSE(finder.isDone());
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
 
     pathStructureValid(result.slice(), 1);
     pathEquals(result.slice(), {1, 2});
@@ -327,7 +332,7 @@ TEST_P(DFSFinderTest, path_depth_1) {
   {
     result.clear();
     // Try again to make sure we stay at non-existing
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_FALSE(hasPath);
     EXPECT_TRUE(result.isEmpty());
     EXPECT_TRUE(finder.isDone());
@@ -351,8 +356,10 @@ TEST_P(DFSFinderTest, path_depth_2) {
   EXPECT_FALSE(finder.isDone());
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 2);
     pathEquals(result.slice(), {1, 2, 3});
 
@@ -362,7 +369,7 @@ TEST_P(DFSFinderTest, path_depth_2) {
   {
     result.clear();
     // Try again to make sure we stay at non-existing
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_FALSE(hasPath);
     EXPECT_TRUE(result.isEmpty());
     EXPECT_TRUE(finder.isDone());
@@ -385,8 +392,10 @@ TEST_P(DFSFinderTest, path_depth_3) {
   EXPECT_FALSE(finder.isDone());
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 3);
     pathEquals(result.slice(), {1, 2, 3, 4});
 
@@ -396,7 +405,7 @@ TEST_P(DFSFinderTest, path_depth_3) {
   {
     result.clear();
     // Try again to make sure we stay at non-existing
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_FALSE(hasPath);
     EXPECT_TRUE(result.isEmpty());
     EXPECT_TRUE(finder.isDone());
@@ -420,24 +429,30 @@ TEST_P(DFSFinderTest, path_diamond) {
   EXPECT_FALSE(finder.isDone());
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 2);
 
     EXPECT_FALSE(finder.isDone());
   }
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 2);
 
     EXPECT_FALSE(finder.isDone());
   }
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 2);
 
     EXPECT_TRUE(finder.isDone());
@@ -446,7 +461,7 @@ TEST_P(DFSFinderTest, path_diamond) {
   {
     result.clear();
     // Try again to make sure we stay at non-existing
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_FALSE(hasPath);
     EXPECT_TRUE(result.isEmpty());
     EXPECT_TRUE(finder.isDone());
@@ -469,8 +484,10 @@ TEST_P(DFSFinderTest, path_depth_1_to_2) {
   EXPECT_FALSE(finder.isDone());
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 1);
     pathEquals(result.slice(), {10, 12});
 
@@ -479,26 +496,32 @@ TEST_P(DFSFinderTest, path_depth_1_to_2) {
 
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 2);
     pathEquals(result.slice(), {10, 12, 13});
     EXPECT_FALSE(finder.isDone());
   }
 
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 2);
     pathEquals(result.slice(), {10, 12, 11});
     EXPECT_FALSE(finder.isDone());
   }
 
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 1);
     pathEquals(result.slice(), {10, 11});
     EXPECT_TRUE(finder.isDone());
@@ -507,7 +530,7 @@ TEST_P(DFSFinderTest, path_depth_1_to_2) {
   {
     result.clear();
     // Try again to make sure we stay at non-existing
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_FALSE(hasPath);
     EXPECT_TRUE(result.isEmpty());
     EXPECT_TRUE(finder.isDone());
@@ -524,8 +547,10 @@ TEST_P(DFSFinderTest, path_depth_1_to_2_skip) {
   EXPECT_FALSE(finder.isDone());
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 1);
     pathEquals(result.slice(), {10, 12});
 
@@ -541,17 +566,21 @@ TEST_P(DFSFinderTest, path_depth_1_to_2_skip) {
 
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 2);
     pathEquals(result.slice(), {10, 12, 11});
     EXPECT_FALSE(finder.isDone());
   }
 
   {
     result.clear();
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_TRUE(hasPath);
+    hasPath->toVelocyPack(result);
+
     pathStructureValid(result.slice(), 1);
     pathEquals(result.slice(), {10, 11});
     EXPECT_TRUE(finder.isDone());
@@ -560,7 +589,7 @@ TEST_P(DFSFinderTest, path_depth_1_to_2_skip) {
   {
     result.clear();
     // Try again to make sure we stay at non-existing
-    auto hasPath = finder.getNextPath(result);
+    auto hasPath = finder.getNextPath();
     EXPECT_FALSE(hasPath);
     EXPECT_TRUE(result.isEmpty());
     EXPECT_TRUE(finder.isDone());