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_view>: basic_string_view::copy() mistakenly marked constexpr before C++20 P1032R1 #203

Closed
crackedmind opened this issue Oct 24, 2019 · 2 comments · Fixed by #491
Closed
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@crackedmind
Copy link
Contributor

crackedmind commented Oct 24, 2019

Describe the bug
During implementation #50 i found that string_view::copy mistakenly marked constexpr before p1032r1

Command-line test case
I guess all versions with string_view support:

C:\Users\billy\Desktop>type string_view_copy.cpp
#include <string_view>
#include <array>

constexpr auto test_string_view_copy() {
    std::string_view a("oops");
    std::array<char,3> a1 = {'1','2','3'};
    a.copy(a1.data(), 3);
    return a1.size();
}


int main() {
    constexpr auto n = test_string_view_copy();
}

C:\Users\billy\Desktop>cl /EHsc /W4 /WX .\cl /std:c++17 /EHsc /MDd /W4 /WX string_view_copy.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.23.28106.4 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

string_view_copy.cpp
string_view_copy.cpp(13): error C2131: expression did not evaluate to a constant
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.23.28105\include\xstring(1322): note: failure was caused by call of undefined function or one not declared 'constexpr'
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.23.28105\include\xstring(1322): note: see usage of 'std::_Narrow_char_traits<char,int>::copy'

Expected behavior
Something like this

cl /std:c++17 /EHsc /MDd /W4 /WX string_view_copy.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.23.28106.4 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

string_view_copy.cpp
string_view_copy.cpp(4): error C3615: constexpr function 'test_string_view_copy' cannot result in a constant expression
string_view_copy.cpp(7): note: failure was caused by call of undefined function or one not declared 'constexpr'
string_view_copy.cpp(7): note: see usage of 'std::basic_string_view<char,std::char_traits<char>>::copy'
string_view_copy.cpp(13): error C2131: expression did not evaluate to a constant
string_view_copy.cpp(13): note: function violates 'constexpr' rules or has errors
string_view_copy.cpp(13): note: see usage of 'test_string_view_copy'

Additional context
Add any other context about the problem here.

@StephanTLavavej StephanTLavavej changed the title string_view::copy mistakenly marked constexpr before p1032r1 <string_view>: basic_string_view::copy() mistakenly marked constexpr before C++20 P1032R1 Oct 24, 2019
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Oct 24, 2019
@StephanTLavavej
Copy link
Member

Great catch! We should define _CONSTEXPR20 after

STL/stl/inc/yvals_core.h

Lines 445 to 450 in 447f879

// C++17 constexpr additions
#if _HAS_CXX17
#define _CONSTEXPR17 constexpr
#else // ^^^ has C++17 constexpr additions / no C++17 constexpr additions vvv
#define _CONSTEXPR17 inline
#endif // _HAS_CXX17

and use it in both basic_string_view::copy() and our non-Standard extension basic_string_view::_Copy_s():

STL/stl/inc/xstring

Lines 1320 to 1335 in 447f879

constexpr size_type copy(_Out_writes_(_Count) _Elem* const _Ptr, size_type _Count, const size_type _Off = 0) const {
// copy [_Off, _Off + Count) to [_Ptr, _Ptr + _Count)
_Check_offset(_Off);
_Count = _Clamp_suffix_size(_Off, _Count);
_Traits::copy(_Ptr, _Mydata + _Off, _Count);
return _Count;
}
_Pre_satisfies_(_Dest_size >= _Count) constexpr size_type _Copy_s(_Out_writes_all_(_Dest_size) _Elem* const _Dest,
const size_type _Dest_size, size_type _Count, const size_type _Off = 0) const {
// copy [_Off, _Off + _Count) to [_Dest, _Dest + _Count)
_Check_offset(_Off);
_Count = _Clamp_suffix_size(_Off, _Count);
_Traits::_Copy_s(_Dest, _Dest_size, _Mydata + _Off, _Count);
return _Count;
}

@BillyONeal
Copy link
Member

Our own _Copy_s we can make constexpr if we want in 17 mode.

This was referenced Dec 10, 2019
@miscco miscco mentioned this issue Apr 11, 2020
4 tasks
@StephanTLavavej StephanTLavavej added fixed Something works now, yay! and removed work in progress labels May 23, 2020
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.

4 participants