[libcxx-commits] [PATCH] D110598: [libc++] Implement P0980R1 (constexpr std::string)
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Apr 4 09:50:26 PDT 2022
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Thanks for the patch! Mostly small comments about forgetting stuff in the tests, however I think we'll need to figure out a better story for the string's invariant inside `constexpr`. I think we agreed on a direction to investigate during our live review, so let's try that and re-evaluate after. Thanks!
================
Comment at: cross-project-tests/debuginfo-tests/llvm-prettyprinters/gdb/mlir-support.cpp:12
mlir::OperationName OperationName("FooOp", &Context);
-mlir::Value Value({reinterpret_cast<void *>(0x8),
+/*mlir::Value Value({reinterpret_cast<void *>(0x8),
mlir::Value::Kind::TrailingOpResult});
----------------
I don't think you intended to upload that.
================
Comment at: libcxx/include/string:2363-2366
+ if (__libcpp_is_constant_evaluated()) {
+ if (__get_long_pointer() != nullptr)
+ __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
+ }
----------------
philnik wrote:
> Quuxplusone wrote:
> > My understanding of this patch is that you're trying to make it an invariant that "`__libcpp_is_constant_evaluated()` implies `__is_long()`." In that case, you shouldn't need this diff, right?
> Yes, this was my plan. But I have to check if it is a nullptr here, because on a moved from object ##__zero()## is called. This way it is not possible using a moved from string in a constexpr context.
Why don't we call `__default_init()` on a moved-from string instead?
Right now, after calling `__zero()` during `constexpr` evaluation, the string is invalid to access, thus we don't have the invariant `__is_long()`. Instead, I think we should make sure that the string satisfies `__is_long()` at all times during constexpr evaluation, which probably means replacing a few calls to `__zero()` by calls to `__default_init()`.
This would allow removing the diff above.
================
Comment at: libcxx/include/string:865-870
+ template <class _Tp, class = __enable_if_t<__can_be_converted_to_string_view<_CharT, _Traits, _Tp>::value &&
+ !__is_same_uncvref<_Tp, basic_string>::value> >
+ _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string& operator=(const _Tp& __t) {
+ __self_view __sv = __t;
+ return assign(__sv);
+ }
----------------
Please consider not reformatting as part of this patch since it's good to keep it as mechanical as possible.
================
Comment at: libcxx/include/string:1935-1936
{
+ if (__libcpp_is_constant_evaluated())
+ __zero();
if (__reserve > max_size())
----------------
What if instead of doing this, we made sure to have the invariants of the string hold before calling `__init`, and then we'd "simply" have to check whether we already have a long string allocated at the beginning of `__init`?
================
Comment at: libcxx/include/string:2098
+ __zero();
+ __r_.first().__l = __str.__r_.first().__l;
__str.__zero();
----------------
This line does not look correct to me. Indeed, if `__str` is a short string, after the change we are accessing `__str`'s long representation instead of its raw representation, which means that we might not be copying some part of the small buffer, if the small buffer is larger than the long representation.
This could happen if we had a very large character type where `2 * sizeof(CharType)` is larger than `sizeof(LongRepresentation)`. In that case, our short string buffer will contain two `CharType`s at least, yet our long representation will be smaller than that. Please add a test to catch this, or explain why that doesn't work (you mentioned a bug related to large character types during live review).
================
Comment at: libcxx/include/string:2550
{
- if (__is_long()) {
+ if (__is_long() && __get_long_pointer() != nullptr) {
__alloc_traits::deallocate(__alloc(), __get_long_pointer(),
----------------
Same here -- if the invariant `__is_long()` held during constexpr evaluation, this `nullptr` check could go away.
================
Comment at: libcxx/include/string:2563
+ if (__libcpp_is_constant_evaluated()) {
+ __str.__zero();
+ } else {
----------------
For instance, I think this is one place where we should call `__default_init()` instead.
================
Comment at: libcxx/include/string:2878-2884
+ if (__libcpp_is_constant_evaluated()) {
+ if (__cap - __sz >= __n)
+ __grow_by_and_replace(__cap, 0, __sz, __pos, 0, __n, __s);
+ else
+ __grow_by_and_replace(__cap, __sz + __n - __cap, __sz, __pos, 0, __n, __s);
+ return *this;
+ }
----------------
I'd like to understand why this diff is needed. What happens if you remove it?
================
Comment at: libcxx/include/string:3093-3096
+ if (__libcpp_is_constant_evaluated()) {
+ __grow_by_and_replace(__cap, 0, __sz, __pos, __n1, __n2, __s);
+ return *this;
+ }
----------------
Same question here -- why is this required?
================
Comment at: libcxx/include/string:4130-4131
return false;
+ if (__libcpp_is_constant_evaluated())
+ return true;
if (capacity() < __min_cap - 1)
----------------
If we never put the string in an invalid state inside `constexpr`, is it possible to remove this diff entirely?
================
Comment at: libcxx/include/string:4372-4373
typename _String::size_type __rhs_sz = __rhs.size();
+ __r.__deallocate_constexpr();
__r.__init(__lhs.data(), __lhs_sz, __lhs_sz + __rhs_sz);
__r.append(__rhs.data(), __rhs_sz);
----------------
Could we investigate adding a private constructor that allows passing a capacity to pre-allocate, so this could be implemented like this:
```
using _String = basic_string<_CharT, _Traits, _Allocator>;
typename _String::size_type __lhs_sz = __lhs.size();
typename _String::size_type __rhs_sz = __rhs.size();
_String __r(_String::__alloc_traits::select_on_container_copy_construction(__lhs.get_allocator()), __capacity_tag{}, __lhs_sz + __rhs_sz);
__r.append(__lhs.data(), __lhs_sz);
__r.append(__rhs.data(), __rhs_sz);
return __r;
```
Can we please do this as a separate review? That should be easy to split out.
================
Comment at: libcxx/test/std/strings/basic.string/string.cons/default_noexcept.pass.cpp:26-37
{
typedef std::string C;
static_assert(std::is_nothrow_default_constructible<C>::value, "");
}
{
typedef std::basic_string<char, std::char_traits<char>, test_allocator<char>> C;
static_assert(std::is_nothrow_default_constructible<C>::value, "");
----------------
Unless I am mistaken, we don't seem to have any test for `basic_string::basic_string()` that runs at runtime. This is kind of crazy.
As a different review, can you please move this test to `default.pass.cpp`, make it run the default constructor at runtime, and prepare it for constexpr-friendliness as well? That way, this diff can look like the other ones where you basically uncomment `static_assert(test());`.
================
Comment at: libcxx/test/std/strings/basic.string/string.cons/dtor_noexcept.pass.cpp:13
-// ~basic_string() // implied noexcept;
+// ~basic_string() // implied noexcept; // constexpr since C++20
----------------
Same thing here -- we don't have a runtime test for the string destructor outside of the static variable below. Please add one -- this can be done in the same review as the default ctor.
You could use a special allocator and check that `deallocate()` has been called after the string is destroyed.
================
Comment at: libcxx/test/std/strings/basic.string/string.iterators/iterators.pass.cpp:81
test();
#if defined(__cpp_lib_constexpr_string) && __cpp_lib_constexpr_string >= 201907L
static_assert(test());
----------------
I think this needs to change to `TEST_STD_VER > 17`?
Please also grep for `__cpp_lib_constexpr_string` after applying your patch just to make sure there aren't any stray ones left where they shouldn't be.
================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_T_size_size.pass.cpp:1867-1898
// static_assert(test0<S, SV>());
// static_assert(test1<S, SV>());
// static_assert(test2<S, SV>());
// static_assert(test3<S, SV>());
// static_assert(test4<S, SV>());
// static_assert(test5<S, SV>());
// static_assert(test6<S, SV>());
----------------
I think you missed those.
I'm also not a big fan of the 2k lines of test here, but this has nothing to do with this review.
================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_insert/size_string_size_size.pass.cpp:1827-1857
// static_assert(test0<S>());
// static_assert(test1<S>());
// static_assert(test2<S>());
// static_assert(test3<S>());
// static_assert(test4<S>());
// static_assert(test5<S>());
// static_assert(test6<S>());
----------------
You forgot those too! Please grep for `// static_assert` in the test suite just to make sure you haven't missed other ones.
================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/iter_iter_iter_iter.pass.cpp:1060-1069
// static_assert(test0<S>());
// static_assert(test1<S>());
// static_assert(test2<S>());
// static_assert(test3<S>());
// static_assert(test4<S>());
// static_assert(test5<S>());
// static_assert(test6<S>());
----------------
Here!
================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/iter_iter_pointer_size.pass.cpp:995-1006
#if TEST_STD_VER > 17
// static_assert(test0<S>());
// static_assert(test1<S>());
// static_assert(test2<S>());
// static_assert(test3<S>());
// static_assert(test4<S>());
// static_assert(test5<S>());
----------------
Here!
================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/size_size_T_size_size.pass.cpp:6079
#if TEST_STD_VER > 17
// static_assert(test0<S, SV>());
// static_assert(test1<S, SV>());
----------------
Here!
================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/size_size_pointer_size.pass.cpp:1340
#if TEST_STD_VER > 17
// static_assert(test0<S>());
// static_assert(test1<S>());
----------------
Here!
================
Comment at: libcxx/test/std/strings/basic.string/string.modifiers/string_replace/size_size_string_size_size.pass.cpp:6069
// static_assert(test34<S>());
// static_assert(test35<S>());
// static_assert(test36<S>());
----------------
Here!
Wow, 6k lines of test. I'm sure they are all 100% relevant -_-.
================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.io/get_line.pass.cpp:14
// getline(basic_istream<charT,traits>& is,
-// basic_string<charT,traits,Allocator>& str);
+// basic_string<charT,traits,Allocator>& str); // constexpr since C++20
----------------
I don't think you meant to make this one constexpr.
================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.io/get_line_delim.pass.cpp:14
// getline(basic_istream<charT,traits>& is,
-// basic_string<charT,traits,Allocator>& str, charT delim);
+// basic_string<charT,traits,Allocator>& str, charT delim); // constexpr since C++20
----------------
Same, not constexpr.
================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.io/get_line_delim_rv.pass.cpp:14
// getline(basic_istream<charT,traits>&& is,
-// basic_string<charT,traits,Allocator>& str, charT delim);
+// basic_string<charT,traits,Allocator>& str, charT delim); // constexpr since C++20
----------------
Same!
================
Comment at: libcxx/test/std/strings/basic.string/string.nonmembers/string.io/get_line_rv.pass.cpp:14
// getline(basic_istream<charT,traits>&& is,
-// basic_string<charT,traits,Allocator>& str);
+// basic_string<charT,traits,Allocator>& str); // constexpr since C++20
----------------
Same, and for all `string.io` probably.
================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_compare/size_size_T_size_size.pass.cpp:5955
int main(int, char**)
{
----------------
We are missing some constexpr-ification here for sure.
================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_compare/size_size_pointer_size.pass.cpp:1335
test11<S>();
// static_assert(test0<S>());
----------------
Missed these ones!
================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_compare/size_size_string_size_size.pass.cpp:6041
// static_assert(test30<S>());
// static_assert(test31<S>());
// static_assert(test32<S>());
----------------
Missed!
================
Comment at: libcxx/test/std/strings/basic.string/string.ops/string_find.last.not.of/string_view_size.pass.cpp:154-157
// typedef std::basic_string<char, std::char_traits<char>, min_allocator<char>> S;
// typedef std::string_view SV;
// test0<S, SV>();
// test1<S, SV>();
----------------
Can you try uncommenting this and seeing what happens?
================
Comment at: libcxx/test/support/constexpr_char_traits.h:121
{
- assert(__s2 < __s1 || __s2 >= __s1+__n);
+ if (!TEST_IS_CONSTANT_EVALUATED)
+ assert(__s2 < __s1 || __s2 >= __s1+__n);
----------------
`// fails in constexpr because we might be comparing unrelated pointers`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110598/new/
https://reviews.llvm.org/D110598
More information about the libcxx-commits
mailing list