future: use "(T(...))" instead of "{T(...)}" in uninitialized_set() #2047
Conversation
|
a reproducer can be found at https://godbolt.org/z/e3qTo1fWf |
tchaikov
force-pushed
the
clang-18-init
branch
from
dcafb84
to
5e8b9f1
Compare
|
the test failures should be addressed by #2048 |
|
Please rebase. |
tchaikov
force-pushed
the
clang-18-init
branch
2 times, most recently
from
148f047
to
3d833dd
Compare
|
@avikivity hi Avi, rebased. |
|
I don't understand the solution. Why is parentheses more restrictive? Here's an alternate solution that restricts the range constructor more fully and so causes the compiler to prefer the move constructor: https://godbolt.org/z/azKa57q79. I'm not saying my solution is better (it's a good idea to restrict the range constructor regardless), but I want to understand why the compiler prefers the range ctor. At least it should complain about ambiguity. Maybe it's a compiler bug? btw, even better is to remove the range constructor, other containers don't have it. Use boost::ranges::copy or similar. |
it bends the type being constructed to address a bug in the caller site, which intends to use the braced-init-list to construct an instance of
Clang prefers the list initialization by implementing a recent fix in the standard. see https://cplusplus.github.io/CWG/issues/2137.html , see also https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2638 for an example. both Clang trunk and GCC trunk implement this proposal. see https://godbolt.org/z/nzK6jzrYv
agreed. but, IMHO, we should not prohibit the type of the future value from having a constructor accepting |
Ah, now I understand (and agree). Please expand the changelog to explain the point about initializer_list. |
in `uninitialized_wrapper_base::uninitialized_set()`, we initialize
the value of a future using
```c++
new (&_v.value) maybe_wrap_ref<T>{T(std::forward<U>(vs)...)};
```
this has been working fine even if `T` provides a constructor
accepting a C++20 range -- the move constructor is picked as
expected.
but the lastest clang (920bb5430a96c346f7afcbe288e3546513b58012)
starts trying to match the constructor of `maybe_wrap_ref<T>`
which accepts a C++20 range, and fails like:
```
/home/kefu/dev/scylladb/utils/chunked_vector.hh:110:38: note: in instantiation of function template specialization 'utils::chunked_vector<int>::chunked_vector<__gnu_cxx::__normal_iterator<const utils::chunked_vector<int> *, std::vector<utils::chunked_vector<int>>>>' requested here
110 | chunked_vector(const Range& r) : chunked_vector(r.begin(), r.end()) {}
| ^
/home/kefu/dev/scylladb/seastar/include/seastar/core/future.hh:301:43: note: in instantiation of function template specialization 'utils::chunked_vector<int>::chunked_vector<std::vector<utils::chunked_vector<int>>>' requested here
301 | new (&_v.value) maybe_wrap_ref<T>{T(std::forward<U>(vs)...)};
| ^
/home/kefu/dev/scylladb/seastar/include/seastar/core/future.hh:615:15: note: in instantiation of function template specialization 'seastar::internal::uninitialized_wrapper_base<std::vector<utils::chunked_vector<int>>, false>::uninitialized_set<std::vector<utils::chunked_vector<int>>>' requested here
615 | this->uninitialized_set(std::forward<A>(a)...);
| ^
/home/kefu/dev/scylladb/seastar/include/seastar/core/future.hh:1253:56: note: in instantiation of function template specialization 'seastar::future_state<std::vector<utils::chunked_vector<int>>>::future_state<std::vector<utils::chunked_vector<int>>>' requested here
1253 | future(ready_future_marker m, A&&... a) noexcept : _state(m, std::forward<A>(a)...) { }
| ^
/home/kefu/dev/scylladb/seastar/include/seastar/core/future.hh:1934:12: note: in instantiation of function template specialization 'seastar::future<std::vector<utils::chunked_vector<int>>>::future<std::vector<utils::chunked_vector<int>>>' requested here
1934 | return future<T>(ready_future_marker(), std::forward<A>(value)...);
| ^
```
where, `T` is std::vector<chunked_vector<int>>`, and the code
which fails to compile is:
```c++
std::vector<utils::chunked_vector<int>> vec;
return make_ready_future<std::vector<utils::chunked_vector<int>>>(std::move(vec));`
````
where compiler is imagining something like:
```c++
std::vector<utils::chunked_vector<int>> vc{a_chunked_vector};
```
but what we are trying to tell it is:
```c++
std::vector<utils::chunked_vector<int>> vc{a_vector_of_chunked_vector};
```
unfortunately, the compiler hit a brick wall. the reason is that
Clang merged a fix of
llvm/llvm-project@9247013
to implement a proposal of
https://cplusplus.github.io/CWG/issues/2137.html, regarding initializing
object with initializer list. see also
https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2638
for an example, which hasn't been included by
https://eel.is/c++draft/dcl.init.list#3.7 at the time of writing though.
both Clang and GCC trunk have implemented the proposed change in C++
standard.
so this issue above is a generalized use form `std::initializer_list`,
and it actually impacts the initialization of all future types accepting
accept `std::initializer_list`.
in this change, let's use '()' instead of '{}', to avoid the resolution
to the constructor from `std::initializer_list`, and to ensure the
compiler picks the right constructor.
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
tchaikov
force-pushed
the
clang-18-init
branch
from
3d833dd
to
b50d7bf
Compare
|
@avikivity thank you for your review. updated the commit message and repushed. could you take another look? |
in
uninitialized_wrapper_base::uninitialized_set(), we initializethe value of a future using
this has been working fine even if
Tprovides a constructoraccepting a C++20 range -- the move constructor is picked as
expected.
but the lastest clang (920bb5430a96c346f7afcbe288e3546513b58012)
starts trying to match the constructor of
maybe_wrap_ref<T>which accepts a C++20 range, and fails like:
where,
Tis std::vector<chunked_vector>`, and the codewhich fails to compile is:
where compiler is imagining something like:
std::vector<utils::chunked_vector<int>> vc{a_chunked_vector};but what we are trying to tell it is:
std::vector<utils::chunked_vector<int>> vc{a_vector_of_chunked_vector};unfortunately, the compiler hit a brick wall. the reason is that
Clang merged a fix of
llvm/llvm-project@9247013
to implement a proposal of
https://cplusplus.github.io/CWG/issues/2137.html, regarding initializing
object with initializer list. see also
https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2638
for an example, which hasn't been included by
https://eel.is/c++draft/dcl.init.list#3.7 at the time of writing though.
both Clang and GCC trunk have implemented the proposed change in C++
standard.
so this issue above is a generalized use form
std::initializer_list,and it actually impacts the initialization of all future types accepting
accept
std::initializer_list.in this change, let's use '()' instead of '{}', to avoid the resolution
to the constructor from
std::initializer_list, and to ensure thecompiler picks the right constructor.