refactor: return OpResult in DbSlice::AddOrFind instead of throwing std::bad_alloc #2427

kostasrim · 2024-01-16T19:13:29Z

resolve comments in #2379

return OpResult in AddOrFind instead of throwing bad_alloc

… bad_alloc

chakaz · 2024-01-16T19:54:03Z

src/server/bitops_family.cc

+  if (!op_res) {
+    return op_res.status();
+  }
+  auto add_res = std::move(*op_res);


Suggested change

auto add_res = std::move(*op_res);

auto& add_res = *op_res;

chakaz · 2024-01-16T19:54:50Z

src/server/bitops_family.cc

+  if (!add_res.is_new) {
+    if (add_res.it->second.ObjType() != OBJ_STRING) {
+      return OpStatus::WRONG_TYPE;


Suggested change

if (!add_res.is_new) {

if (add_res.it->second.ObjType() != OBJ_STRING) {

return OpStatus::WRONG_TYPE;

if (!add_res.is_new && add_res.it->second.ObjType() != OBJ_STRING) {

return OpStatus::WRONG_TYPE;

chakaz · 2024-01-16T19:58:32Z

src/server/db_slice.cc

+  if (!op_result) {
+    return op_result.status();
+  }
+  auto res = std::move(*op_result);


Suggested change

auto res = std::move(*op_result);

auto& res = *op_result;

chakaz · 2024-01-16T20:01:14Z

src/server/db_slice.cc

@@ -495,7 +495,8 @@ OpResult<pair<PrimeConstIterator, unsigned>> DbSlice::FindFirstReadOnly(const Co
  return OpStatus::KEY_NOTFOUND;
 }

-DbSlice::AddOrFindResult DbSlice::AddOrFind(const Context& cntx, string_view key) noexcept(false) {
+OpResult<DbSlice::AddOrFindResult> DbSlice::AddOrFind(const Context& cntx,
+                                                      string_view key) noexcept(false) {


can we change this to noexcept(true) now? or just noexcept?
Or just remove this, as we never throw in other places anyway?

just noexcept will suffice but if we omit it completely it's also ok I guess. I keep conditional noexcept only when there is type deduction

chakaz · 2024-01-16T20:01:30Z

src/server/db_slice.cc

-  auto res = AddOrUpdateInternal(cntx, key, std::move(obj), expire_at_ms, false);
+OpResult<DbSlice::ItAndUpdater> DbSlice::AddNew(const Context& cntx, string_view key,
+                                                PrimeValue obj,
+                                                uint64_t expire_at_ms) noexcept(false) {


same question here

chakaz · 2024-01-16T20:01:40Z

src/server/db_slice.cc

+                                                                std::string_view key,
+                                                                PrimeValue obj,
+                                                                uint64_t expire_at_ms,
+                                                                bool force_update) noexcept(false) {


ditto, and in other places :)

chakaz · 2024-01-16T20:04:56Z

src/server/generic_family.cc

+  if (!op_result) {
+    return op_result.status();
+  }
+  auto add_res = std::move(*op_result);


Suggested change

auto add_res = std::move(*op_result);

auto* add_res = *op_result;

chakaz · 2024-01-16T20:05:39Z

src/server/hll_family.cc

+  if (!op_res) {
+    return op_res.status();
+  }
+  auto res = std::move(*op_res);


Suggested change

auto res = std::move(*op_res);

auto& res = *op_res;

chakaz · 2024-01-16T20:06:07Z

src/server/hll_family.cc

    }
+    auto res = std::move(*op_res);


Suggested change

auto res = std::move(*op_res);

auto& res = *op_res;

chakaz · 2024-01-16T20:06:20Z

src/server/hset_family.cc

  }
+  auto add_res = std::move(*op_res);


Suggested change

auto add_res = std::move(*op_res);

auto& add_res = *op_res;

I'm tired of writing these, also in other places in this PR :)

chakaz · 2024-01-21T06:29:20Z

src/server/db_slice.cc

+                                     .key = key})};
+  }
+  auto status = res.status();
+  CHECK_EQ(status == OpStatus::KEY_NOTFOUND || status == OpStatus::OUT_OF_MEMORY, true);


Suggested change

CHECK_EQ(status == OpStatus::KEY_NOTFOUND || status == OpStatus::OUT_OF_MEMORY, true);

CHECK(status == OpStatus::KEY_NOTFOUND || status == OpStatus::OUT_OF_MEMORY) << status;

chakaz · 2024-01-21T06:32:44Z

src/server/dragonfly_test.cc

@@ -541,19 +541,19 @@ TEST_F(DflyEngineTest, Bug496) {
        db.RegisterOnChange([&cb_hits](DbIndex, const DbSlice::ChangeReq&) { cb_hits++; });

    {
-      auto res = db.AddOrFind({}, "key-1");
+      auto res = std::move(*db.AddOrFind({}, "key-1"));


Here's a proposal:
Maybe add V&& operator*() &&; to OpResult, and then we won't have to call std::move() on what seems like rvalue types? We do it quite a lot, so I think it's worth it. That's also how std::optional allows similar things.

chakaz · 2024-01-21T06:42:05Z

src/server/bitops_family.cc

+  if (!op_res) {
+    return op_res.status();
+  }


We have a similar if in many places. For std::status_code we have RETURN_ON_ERR() (see error.h), so how about creating a RETURN_ON_BAD_STATUS() macro?

chakaz

awesome :)

refactor(): return OpResult in DbSlice::AddOrFind instead of throwing…

2e479da

… bad_alloc

kostasrim self-assigned this Jan 16, 2024

kostasrim requested a review from chakaz January 16, 2024 19:13

kostasrim changed the title ~~refactor(): return OpResult in DbSlice::AddOrFind instead of throwing std::bad_alloc~~ refactor: return OpResult in DbSlice::AddOrFind instead of throwing std::bad_alloc Jan 16, 2024

chakaz reviewed Jan 16, 2024

View reviewed changes

kostasrim added 2 commits January 18, 2024 20:13

address gh comments

76c2ff8

Merge branch 'main' into add_or_find_ret

bd86e00

kostasrim requested a review from chakaz January 18, 2024 18:33

chakaz reviewed Jan 21, 2024

View reviewed changes

address gh comments

c5ea571

kostasrim requested a review from chakaz January 23, 2024 11:10

chakaz approved these changes Jan 23, 2024

View reviewed changes

kostasrim merged commit 517be20 into main Jan 23, 2024
10 checks passed

kostasrim deleted the add_or_find_ret branch January 23, 2024 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: return OpResult in DbSlice::AddOrFind instead of throwing std::bad_alloc #2427

refactor: return OpResult in DbSlice::AddOrFind instead of throwing std::bad_alloc #2427

kostasrim commented Jan 16, 2024

chakaz Jan 16, 2024

chakaz Jan 16, 2024

chakaz Jan 16, 2024

chakaz Jan 16, 2024

kostasrim Jan 18, 2024 •

edited

chakaz Jan 16, 2024

chakaz Jan 16, 2024

chakaz Jan 16, 2024

chakaz Jan 16, 2024

chakaz Jan 16, 2024

chakaz Jan 16, 2024

chakaz Jan 16, 2024

chakaz Jan 21, 2024

chakaz Jan 21, 2024

chakaz Jan 21, 2024

chakaz left a comment

	auto add_res = std::move(*op_result);
	auto* add_res = *op_result;

	CHECK_EQ(status == OpStatus::KEY_NOTFOUND \|\| status == OpStatus::OUT_OF_MEMORY, true);
	CHECK(status == OpStatus::KEY_NOTFOUND \|\| status == OpStatus::OUT_OF_MEMORY) << status;

refactor: return OpResult in DbSlice::AddOrFind instead of throwing std::bad_alloc #2427

refactor: return OpResult in DbSlice::AddOrFind instead of throwing std::bad_alloc #2427

Conversation

kostasrim commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Jan 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakaz left a comment

Choose a reason for hiding this comment

kostasrim Jan 18, 2024 •

edited