Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix assignment operator in Optional #720

Merged
merged 12 commits into from
Mar 28, 2025
Merged

Conversation

sfod
Copy link
Contributor

@sfod sfod commented Mar 25, 2025

Issue #, if available:

There are multiple issues with Optional's assignment operators and related tests:

  • template <typename U = T> Optional &operator=(U &&u) is too greedy, so it's used whenever argument type differs from other assignment operators types:

    Optional<Foo> o1;
    Optional<Foo> o2;
    o2 = o1;  // resolves to the "greedy" assignment operator, because o1 is not const ref
    
  • template <typename U = T> Optional<T> &operator=(const Optional<U> &other) and template <typename U = T> Optional<T> &operator=(Optional<U> &&other) use other's private members

    Specializations of a template class are completely unrelated, so they can't access each other's private members.

  • Tests for assignment operators for Optional class check constructors instead.

    Tests use the following expressions to check assignment operators:

    Optional<Foo> o = another_optional;
    

    But constructor is used here.

The issues can be found in the first commit's CI in this PR. For example, the following code from the tests

Aws::Crt::Optional<CopyMoveTester> copyAssignedOptional;
Aws::Crt::Optional<CopyMoveTester> tester = CopyMoveTester();
copyAssignedOptional = tester;

produces these errors:

/root/aws-crt-cpp/include/aws/crt/Optional.h: In instantiation of Aws::Crt::Optional<T>& Aws::Crt::Optional<T>::operator=(U&&) [with U = Aws::Crt::Optional<CopyMoveTester>&; T = CopyMoveTester]:
/root/aws-crt-cpp/tests/OptionalMemorySafetyTest.cpp:219:36:   required from here
/root/aws-crt-cpp/include/aws/crt/Optional.h:45:30: error: no match for operator= (operand types are CopyMoveTester and Aws::Crt::Optional<CopyMoveTester>)
                     *m_value = std::forward<U>(u);

Description of changes:

  • Make greedy assignment operator less greedy by enabling it only if its parameter type is not Optional.
  • Fix member access in other assignment operators.
  • Fix tests.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sfod sfod marked this pull request as ready for review March 26, 2025 20:29
@sfod sfod merged commit 2df6bee into main Mar 28, 2025
63 checks passed
@sfod sfod deleted the fix-optional-assignment-operator branch March 28, 2025 18:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants