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

<string>: Consider improving conformance for char_traits #3735

Closed
achabense opened this issue May 26, 2023 · 4 comments · Fixed by #3739
Closed

<string>: Consider improving conformance for char_traits #3735

achabense opened this issue May 26, 2023 · 4 comments · Fixed by #3739
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@achabense
Copy link
Contributor

achabense commented May 26, 2023

  1. According to the standard(ref), char_traits::eq/lt/to_char_type/to_int_type/eq_int_type/not_eof should take values instead of references as parameters.
    (
    Also in
    https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L305-L327?plain=1
    https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L449-L471?plain=1
    )

    STL/stl/inc/xstring

    Lines 193 to 215 in a621095

    _NODISCARD static constexpr bool eq(const _Elem& _Left, const _Elem& _Right) noexcept {
    return _Left == _Right;
    }
    _NODISCARD static constexpr bool lt(const _Elem& _Left, const _Elem& _Right) noexcept {
    return _Left < _Right;
    }
    _NODISCARD static constexpr _Elem to_char_type(const int_type& _Meta) noexcept {
    return static_cast<_Elem>(_Meta);
    }
    _NODISCARD static constexpr int_type to_int_type(const _Elem& _Ch) noexcept {
    return static_cast<int_type>(_Ch);
    }
    _NODISCARD static constexpr bool eq_int_type(const int_type& _Left, const int_type& _Right) noexcept {
    return _Left == _Right;
    }
    _NODISCARD static constexpr int_type not_eof(const int_type& _Meta) noexcept {
    return _Meta != eof() ? _Meta : !eof();
    }

  2. Also, int_types for char_traits<char16_t>, <char32_t> and <wchar_t> should be uint_least16_t, uint_least32_t and wint_t respectively. Currently they are defined as unsigned short, unsigned int and unsigned short. --They just works now. However, I think it will be better to use aliases directly, or at least add some static_asserts(e.g. static_assert(std::is_same_v<wint_t,unsigned short>).

    STL/stl/inc/xstring

    Lines 337 to 344 in a621095

    template <>
    struct char_traits<char16_t> : _WChar_traits<char16_t> {};
    template <>
    struct char_traits<char32_t> : _Char_traits<char32_t, unsigned int> {};
    template <>
    struct char_traits<wchar_t> : _WChar_traits<wchar_t> {};

    (char_traits<char>,<char8_t> has no such issue)

  3. (off topic) We can safely add noexcept /*strengthened*/ for this function:

    STL/stl/inc/xstring

    Lines 1402 to 1404 in a621095

    _NODISCARD constexpr int compare(_In_z_ const _Elem* const _Ptr) const { // compare [0, _Mysize) with [_Ptr, <null>)
    return compare(basic_string_view(_Ptr));
    }

  4. (off topic; question) What does the template(line2547~2549) do here? _Alloc2 is not used in the body, and _Alloc is directly used as allocator in other member functions.
    (
    Also here; the only another use of this pattern
    https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L2560-L2566?plain=1
    )

    STL/stl/inc/xstring

    Lines 2547 to 2553 in a621095

    #if _HAS_CXX17
    template <class _Alloc2 = _Alloc, enable_if_t<_Is_allocator<_Alloc2>::value, int> = 0>
    #endif // _HAS_CXX17
    _CONSTEXPR20 basic_string(_In_z_ const _Elem* const _Ptr, const _Alloc& _Al)
    : _Mypair(_One_then_variadic_args_t{}, _Al) {
    _Construct<_Construct_strategy::_From_ptr>(_Ptr, _Convert_size<size_type>(_Traits::length(_Ptr)));
    }

@frederick-vs-ja
Copy link
Contributor

4. _Alloc2 is not used in the body, and _Alloc is directly used as allocator in other member functions.

This is needed by SFINAE. In order to constrain this constructor, we need to make it a function template and introduce a template parameter on which substitution may fail.

@achabense
Copy link
Contributor Author

achabense commented May 26, 2023

@frederick-vs-ja Can we simply remove this constraint? The constraint is not used elsewhere other than the two cases(basic_string(const CharT*, const _Alloc& alloc = _Alloc()) and basic_string(size_type, CharT, const _Alloc& alloc = _Alloc())) --_Alloc is the allocator for basic_string. Also, the impl defines separate basic_string(...) and basic_string(..., const _Alloc&) for basic_string(..., const _Alloc& alloc = _Alloc());.

@frederick-vs-ja
Copy link
Contributor

Can we simply remove this constraint?

No. This constraint is added by LWG-3076, which should be considered as a defect report against C++17 (in which CTAD is introduced).

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented May 27, 2023

  1. According to the standard(ref), char_traits::eq/lt/to_char_type/to_int_type/eq_int_type/not_eof should take values instead of references as parameters.

Wow, this is a missed change from WG21-N2349. We need to implement these C++11 changes.

FYI libstdc++ can't make the changes for char and wchar_t specializations due to ABI compatibility (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80810).

Edit: find and one overload of assign take the character argument by reference, which looks weird to me. I've mailed to LWG Chair to submit an LWG issue for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants