Skip to content

Adjust result.h error condition behavior#128

Open
alan-george-lk wants to merge 7 commits into
mainfrom
feature/result_noexcept
Open

Adjust result.h error condition behavior#128
alan-george-lk wants to merge 7 commits into
mainfrom
feature/result_noexcept

Conversation

@alan-george-lk
Copy link
Copy Markdown
Collaborator

@alan-george-lk alan-george-lk commented May 14, 2026

This PR addresses a minor API change around result.h (dropping noexcept label)

Context:

  • Result accessors were previously marked noexcept
  • clang-tidy flagged this as inaccurate because std::get on a std::variant can throw a std::bad_variant_access if the type being accessed doesn't match the position

Considered alternatvies:

  • Keeping noexcept but using something like std::get_if (doesn't throw) --> declined due to dereferencing nullptr to maintain API signature
  • Switching away from std::variant entirely to avoid to avoid this issue --> declined because it still doesn't address the root issue, which is accessing an error result on success and vice-versa
  • Adjusting the API to return std::optional for accessors --> declined as too impactful to API, and conditionals ok() and has_error() already exist, would be redundant

Decision:

  • result.h accessor methods drop noexcept
  • Throw an exception with a message ourselves vs. going through std::get (users can't do much with a generic std::bad_variant_access exception with no message)

Impact:

This only affects users of result.h that were expecting an exception-free experience, which is negligible if not completely nonexistent at this point in the SDK maturity. In the existing version before this PR, the exception would still actually be thrown if the wrong type was accessed, even though it was marked noexcept.

Testing

  • Additional unit tests to cover this flow

@alan-george-lk alan-george-lk marked this pull request as ready for review May 14, 2026 02:01
Copy link
Copy Markdown
Collaborator

@stephen-derosa stephen-derosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice LGTM

Comment thread include/livekit/result.h Outdated

#pragma once

// TODO (AEG) This include (cassert) leaked into downstream cpp examples, removing it here breaks builds.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dang nice find, are you planning on doing this as a part of this work? If not, consider making a ticket to track

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm going to update examples first, then adjust that in this PR, that way there's not 3 total PRs

Comment thread src/tests/unit/test_result.cpp
Copy link
Copy Markdown
Collaborator

@xianshijing-lk xianshijing-lk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, lgtm in general

Comment thread include/livekit/result.h
/// Move the success value out.
///
/// @throws std::logic_error if `ok() == false`.
const T&& value() const&& {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:
do we such overload ? any existing use case ?

I wonder if we should just remove the const T&& / const E&& overloads since they do not bring much practical benefit

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephen-derosa any insight here? I agree not a ton of value for such a small data structure. Gonna assume this was agentic thorough-ness and not intentional

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants