Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

🛠️ for flat_set #4050

Merged
merged 11 commits into from
Sep 27, 2023
Merged

Conversation

achabense
Copy link
Contributor

@achabense achabense commented Sep 25, 2023

  • 2: insert_range should not unconditionally rely on append_range.
  • 4.1&4.2: cleanups for debug checks.
  • 5&6: more test coverages. fix support for uncomparable types.
  • 7: enhance usage of scope guard. (implement invariant safety against exceptions for operator= and replace; erase and insert methods need further investigations)
  • other improvements / cleanups.

@achabense achabense requested a review from a team as a code owner September 25, 2023 09:59
stl/inc/flat_set Show resolved Hide resolved
@@ -420,7 +428,7 @@ public:
return _RANGES equal(_Lhs._Get_cont(), _Rhs._Get_cont());
}

_NODISCARD friend _Synth_three_way_result<_Kty> operator<=>(const _Deriv& _Lhs, const _Deriv& _Rhs) {
_NODISCARD friend auto operator<=>(const _Deriv& _Lhs, const _Deriv& _Rhs) {
Copy link
Contributor Author

@achabense achabense Sep 25, 2023

Choose a reason for hiding this comment

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

This was making test_comparer_application fail to compile.

... though this "fixes" the problem, I'm afraid we need to look into what is actually going on around this.

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not a compiler bug, the code is rejected by both msvc, gcc and clang :|
https://godbolt.org/z/jT6sT4EfT (the error message given by msvc is the strangest one though)

#include<utility>

#define USE_FRIEND

struct myless {
    template<class L, class R>
    auto operator()(const L& lhs, const R& rhs) {
        return lhs < rhs;
    }
};

template<class L, class R = L>
using myless_result = decltype(myless{}(std::declval<L&>(), std::declval<R&>()));

template<class T>
struct wrapper {
    T t;
#if defined USE_FRIEND
#if 1
    // "unexpected token(s) preceding '{'; skipping apparent function body"
    // then other errors.
    friend myless_result<T> operator<(const wrapper& lhs, const wrapper& rhs) {
        return myless{}(lhs.t, rhs.t);
    }
#else
    // will be ok if returns auto.
    friend auto operator<(const wrapper& lhs, const wrapper& rhs) {
        return myless{}(lhs.t, rhs.t);
    }
#endif

#endif // USE_FRIEND
};

#if !defined USE_FRIEND
// will be ok if not friend.
template<class T>
myless_result<T> operator<(const wrapper<T>& lhs, const wrapper<T>& rhs) {
    return myless{}(lhs, rhs);
}
#endif // !USE_FRIEND

struct xcmp {
    bool operator=(const xcmp&)const = delete;
    bool operator<(const xcmp&)const = delete;
};

int main() {
    wrapper<xcmp> test;
}


_Container& _Cont = _Get_cont();
if constexpr (requires { _Cont.append_range(_STD forward<_Rng>(_Range)); }) {
_Cont.append_range(_STD forward<_Rng>(_Range));
Copy link
Contributor Author

@achabense achabense Sep 25, 2023

Choose a reason for hiding this comment

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

append_range should be considered an optimization here.
I notice that insert_range(end,...) (instead of append_range) is unconditionally required for sequence containers. However, I think we don't have to give an extra check for that chance, as it is likely that a container that doesn't support append_range will not support insert_range either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I find these funtions lack forward (required by the standard):

STL/stl/inc/stack

Lines 115 to 117 in 48eedd3

void push_range(_Rng&& _Range) {
if constexpr (requires { c.append_range(_Range); }) {
c.append_range(_Range);

STL/stl/inc/queue

Lines 125 to 127 in 48eedd3

void push_range(_Rng&& _Range) {
if constexpr (requires { c.append_range(_Range); }) {
c.append_range(_Range);

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we forward here? It seems that std::forward merely rejects non-borrowed-range rvalues, which defends dangling.

But we are eagerly consuming ranges here, so it seems to me that we can just use the ranges as lvalues for containers. CC-ing @cor3ntin @ericniebler @CaseyCarter.

}

static constexpr const char* _Msg_not_sorted = _Multi ? "Input was not sorted!" : "Input was not sorted-unique!";
Copy link
Contributor Author

@achabense achabense Sep 25, 2023

Choose a reason for hiding this comment

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

I have no idea how to represent this gracefully :( It might be helpful to an additional version of _STL_ASSERT that repeat error message just like _STL_INTERNAL_CHECK and assert.

Also, these references in static_assert are not correct, as _Base_flat_set is also the base for flat_multiset.

STL/stl/inc/flat_set

Lines 47 to 49 in 6bedc29

static_assert(same_as<_Kty, typename _Container::value_type>,
"The C++ Standard dictates that the Key type must be the "
"same as the container's value type [flatset.overview]");

STL/stl/inc/flat_set

Lines 65 to 66 in 6bedc29

static_assert(random_access_iterator<iterator>, "The C++ Standard forbids containers without random "
"access iterators from being adapted. See [flatset.overview].");

@achabense achabense changed the title Cleanups / bugfixes for flat_set 🛠️ for flat_set Sep 25, 2023
@achabense achabense mentioned this pull request Sep 25, 2023
15 tasks
@StephanTLavavej StephanTLavavej added the flat_meow C++23 container adaptors label Sep 25, 2023
@StephanTLavavej StephanTLavavej self-assigned this Sep 26, 2023
stl/inc/flat_set Outdated Show resolved Hide resolved
// always clears the container (N4950 [flat.set.modifiers]/14 and [flat.multiset.modifiers]/10)
_Clear_scope_guard<_Base_flat_set> _Guard{this};
_Clear_guard<_Base_flat_set, is_nothrow_move_constructible_v<_Container>> _Guard{this};
return _STD move(_Get_cont());
}
void replace(container_type&& _Cont) {
_STL_ASSERT(_Check_sorted(_Cont.cbegin(), _Cont.cend()), _Msg_not_sorted);
Copy link
Contributor Author

@achabense achabense Sep 26, 2023

Choose a reason for hiding this comment

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

I'd really hope to add noexcept enhancement for replace, but I have no idea how to deal with this _STL_ASSERT :(

return _Emplace(_STD move(_Val));
}
template <class _Other>
template <_Different_from<_Kty> _Other>
Copy link
Contributor Author

@achabense achabense Sep 26, 2023

Choose a reason for hiding this comment

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

For example, without this constraint, when the comparer is transparent inputs like non-const _Kty& will go into this function. The case will be correctly handled by _Emplace, but I think it will be better just fall to insert(const _Kty&).

stl/inc/flat_set Outdated Show resolved Hide resolved
stl/inc/flat_set Outdated Show resolved Hide resolved
stl/inc/flat_set Outdated Show resolved Hide resolved
tests/std/tests/P1222R4_flat_set/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P1222R4_flat_set/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej merged commit e23a4e2 into microsoft:feature/flat_set Sep 27, 2023
@StephanTLavavej
Copy link
Member

Thanks for these fixes and improvements! 🥞

@achabense achabense deleted the _Flat_set_3 branch September 28, 2023 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flat_meow C++23 container adaptors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants