feat: prefer result over throwing exceptions #70

k-wasniowski · 2025-10-13T15:08:10Z

No description provided.

include/sframe/result.h

bifurcation · 2025-10-13T20:43:49Z

include/sframe/result.h

+  T MoveValue() { return std::move(*value_); }
+
+private:
+  SFrameError error_;


Following the example of Rust's Result<T, E>, it would be slightly tempting to be generic over the error type. OTOH, Rust crates almost always specialize to Result<T> anyway, so we could just skip the intermediate step.

This should also be std::optional and we should remove the none variant, though see the comments about variant below.

I think we need to keep none variant due to that an caller could call MoveError/error even though it's success. In that case we need to construct an empty error which reason should rather preferably be none

Is the goal here to emulate rust's Result? Or to emulate RTCError from webrtc? I thought kacper had said the latter, which explains these choices. I don't have a strong opinion one way or another, but would say that we should definitely choose one and be consistent with that.

EDIT: I actually intended this to go under the discussion for ok vs is_ok, but I guess it's a question in general.

bifurcation · 2025-10-13T20:48:30Z

include/sframe/result.h

+
+private:
+  SFrameError error_;
+  std::optional<T> value_;


It would be more correct to use std::variant<T, SFrameError> here, since it would force you to have exactly one of value/error populated, not neither or both. The syntax for accessing variants is hideous (std::visit, etc.), but we could isolate inside of this class's methods so that it didn't bother calling code.

bifurcation · 2025-10-13T20:50:00Z

include/sframe/result.h

+
+  bool ok() const { return error_.ok(); }
+
+  const T& value() const { return *value_; }


Strongly prefer std::get over operator* because it throws if the optional is not present. That won't be any better if built without exceptions, but in other environments, it fails safer.

Slightly inclined to mimic the Rust syntax here, with unwrap to request the value and unwrwap_err to request the error.

Switched all places to std::get

bifurcation · 2025-10-13T20:51:30Z

include/sframe/result.h

+  template<typename U>
+  friend class Result;


Nit: Consider moving to the bottom.

Do we actually need this?

Good catch it's not, leftover from the original on which i was basing. Removed

bifurcation · 2025-10-13T20:53:14Z

include/sframe/result.h

+
+  const T& value() const { return *value_; }
+
+  T& value() { return *value_; }


Also add T&& value() &&?

Not sure if we need this access operator at all. Since i would rather use MoveValue() in all places. Maybe it's better to remove MoveValue, and make only T&& value() && ??

bifurcation · 2025-10-13T20:54:49Z

include/sframe/result.h

+    return std::move(error_);
+  }
+
+  bool ok() const { return error_.ok(); }


Nit: Mimicking Rust would have this as is_ok(), and possibly a parallel is_err().

Replaced existing ok() with is_ok, and added is_err, not sure though if we need the latter

bifurcation · 2025-10-13T20:57:36Z

include/sframe/result.h

+// Helper functions to create Results
+template<typename T>
+Result<T>
+Ok(const T& value)


I like where you're going with this, but to match C++ style, these should probably be ok and err. You could also make them static functions on Result<T>, which would give you Result<T>::ok(value), but unfortunately, you would have to include the <T> because of the limits of C++ type inference.

... though now I see that you need to do that for Err anyway, so yeah, I would probably go with Result<T>::err().

bifurcation · 2025-10-13T21:06:35Z

src/header.cpp

+      // Handle error - for now, throw or return default
+      throw std::runtime_error("Failed to decode uint");


You might want to define a macro for this, something like:

#defin CHECK(rv) if (!(rv).ok()) { return rv; }

See also: https://searchfox.org/firefox-main/search?q=%23define+ENSURE&path=&case=true&regexp=false

bbaldino · 2025-10-15T17:34:59Z

include/sframe/result.h

+
+private:
+  SFrameErrorType type_ = SFrameErrorType::none;
+  std::string message_;


Part of the "which model should we emulate" discussion, but it appears that the message is optional so I wonder if this should be optional?

bbaldino · 2025-10-15T17:37:29Z

include/sframe/result.h

+  T MoveValue() { return std::move(std::get<T>(data_)); }
+
+private:
+  std::variant<SFrameError, T> data_;


An example of being consistent: if we are going to emulate rust's result, I think we should model the type first here.

bbaldino · 2025-10-15T17:39:49Z

src/header.cpp

    if (!is_length) {
      // Nothing to read; value is already in config byte
-      return { value_or_length, data };
+      return Result<std::tuple<uint64_t, input_bytes>>::ok(


Can we omit the explicit template types in these return values since they're specified in the signature?

bbaldino · 2025-10-15T17:41:47Z

src/header.cpp

-    const auto value = decode_uint(data.subspan(0, size));
+    auto value_result = decode_uint(data.subspan(0, size));
+    if (!value_result.is_ok()) {
+      return value_result.MoveError();


I don't think I'm following the need for the MoveError method/call here (probably just my c++ skills are too eroded). Why can't we just return value_result here?

value_result have type sframe::Result<uint64_t>
this function expects to return Result<std::tuple<uint64_t, input_bytes>> so it doesnt match.

I return here with MoveError(), to trigger implicit error creation from move constructor.

To be able to return value_result i would need to define conversion between those two return types.

Ah ha, ok, didn't notice there was a conversion happening there.

k-wasniowski · 2025-10-30T11:08:41Z

include/sframe/map.h

 public:
  template<class... Args>
-  void emplace(Args&&... args)
+  Result<void> emplace(Args&&... args)


This changes the signature of the method. Probably will need to define new method to perform injecton

feat: prefer result over throwing exceptions

70a9a7f

bifurcation reviewed Oct 13, 2025

View reviewed changes

k-wasniowski added 2 commits October 14, 2025 09:31

address comments

ece6926

prefer is_ok over ok

8ee5b1f

bbaldino reviewed Oct 15, 2025

View reviewed changes

k-wasniowski added 2 commits October 16, 2025 10:54

all transformed to result

28fa8b6

refactor to use result everywhere

33809d1

k-wasniowski commented Oct 30, 2025

View reviewed changes

k-wasniowski added 2 commits January 16, 2026 18:14

updated to use result

0ae6e99

ready

5a774bc


		bool ok() const { return error_.ok(); }

		const T& value() const { return *value_; }


		const T& value() const { return *value_; }

		T& value() { return *value_; }

		// Handle error - for now, throw or return default
		throw std::runtime_error("Failed to decode uint");

feat: prefer result over throwing exceptions #70

Are you sure you want to change the base?

feat: prefer result over throwing exceptions #70

Uh oh!

Conversation

k-wasniowski commented Oct 13, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbaldino Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bbaldino Oct 15, 2025 •

edited

Loading