Skip to content

Fixes and code cleanup for include/pfn/expected.hpp #189

Open
Bronek wants to merge 13 commits intomainfrom
bronek/pfn_expected_cleanup
Open

Fixes and code cleanup for include/pfn/expected.hpp #189
Bronek wants to merge 13 commits intomainfrom
bronek/pfn_expected_cleanup

Conversation

@Bronek
Copy link
Copy Markdown
Member

@Bronek Bronek commented May 7, 2026

Changes summary:

  • Constraint on converting assignment wrongly used is_assignable_v<T, U>, SFINAE-disabling the template for scalar T. Fixed and tightened noexcept extension in the same.
  • unexpected::operator== now uses error() instead of the private member, fixing heterogeneous comparison. Added a noexcept extension in the same.
  • Added/improved comments, added reference to P3798 has_error()
  • Improved unit tests coverage, fixed typos in test names
  • Removed dead code _can_convert in expected<void, E>

@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
include/pfn/expected.hpp 71.4% 6 Missing ⚠️

📢 Thoughts on this report? Let us know!

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented May 7, 2026

🤖 Augment PR Summary

Summary: Cleans up and fixes the pfn::expected polyfill implementation, with an emphasis on correct constraints and improved noexcept propagation (as an explicit extension).

Changes:

  • Added a header-level note documenting the polyfill’s intent and the project’s “derived noexcept” extension policy.
  • Fixed converting assignment constraints by using std::is_assignable_v<T&, U> (avoids unintended SFINAE for scalar T).
  • Updated unexpected::operator== to compare via error() (enables heterogeneous comparisons) and extended its noexcept per the extension policy.
  • Tightened expected conversion-assignment noexcept clauses to reflect assign/construct properties more directly.
  • Annotated has_error() with a reference to P3798.
  • Removed unused/dead internal helper code in expected<void, E>.
  • Expanded unit tests for heterogeneous unexpected equality and for assignment/swap noexcept behavior across more type combinations.
  • Enhanced helper test types to model throwing assignment operators (used to validate propagation behavior).

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

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

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread include/pfn/expected.hpp
template <class E2> constexpr friend bool operator==(unexpected const &x, unexpected<E2> const &y)
template <class E2>
constexpr friend bool operator==(unexpected const &x, unexpected<E2> const &y) //
noexcept(noexcept(static_cast<bool>(x.error() == y.error()))) // extension
Copy link
Copy Markdown

@augmentcode augmentcode Bot May 7, 2026

Choose a reason for hiding this comment

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

include/pfn/expected.hpp:149 — The noexcept computation uses static_cast<bool>(x.error() == y.error()); this can succeed even when the comparison result is only explicitly convertible to bool, while the return statement still requires an implicit conversion to bool. Consider aligning the noexcept/participation check with implicit convertibility to avoid surprising hard errors in some generic contexts.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

@Bronek Bronek force-pushed the bronek/pfn_expected_cleanup branch from a784bcc to 553f03c Compare May 8, 2026 16:06
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 8, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
19.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

1 participant