New issue
Advanced search Search tips

Issue 788913 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Feature



Sign in to add a comment

Add initializer_list constructor to base::span

Project Member Reported by reillyg@chromium.org, Nov 27 2017

Issue description

Consider adding an initializer_list constructor to base::span so that a base::span<const T> can be implicitly constructed from an std::initializer_list<T>.

This would allow the following:

void Foo(base::span<const int> foo) {
  // Use |foo|.
}

Foo({1, 2, 3, 4});

This is useful when Foo is called with various sets of values which are all constexprs and so the array allocation necessary if it took const std::vector<int>& can be avoided.
 

Comment 1 by pkl@chromium.org, Dec 18 2017

Can you suggest an owner for this?
Can this be Available w/o an Owner?
Cc: jdoerrie@chromium.org
Adding jdoerrie@ who has been maintaining base::span. Since this is class mirrors one being proposed for inclusion in the STL our own custom constructors may not be welcome.
dcheng@ and I discussed this offline some time ago. We decided to not add a std::initializer_list<T> constructor for now, as it is not mentioned in the current standardization proposal: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0122r5.pdf I'm happy to add one in case the proposal gets updated in the future. 

In the meantime you could add an overload for Foo that dispatches to the span version, e.g. like this:

void Foo(std::initializer_list<int> list) {
  Foo(base::make_span(list.begin(), list.end()));
}

This is the pattern we currently use for base::Value. Also see the related discussion on the corresponding base::Value CL: https://chromium-review.googlesource.com/c/chromium/src/+/623707#message-616ca8be5d5f971227fb678fdec094855b067339
Status: WontFix (was: Untriaged)
Marking this as WontFix for now.
Cc: -jdoerrie@chromium.org
Owner: jdoerrie@chromium.org
Status: Started (was: WontFix)
The std::span proposal got updated [1] and allows construction from std::initializer_list, among others. dcheng@ and I discussed adding the constructors in https://crrev.com/c/976123/9/base/containers/span.h#199.

I will send out a CL that implements this change.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82d9545a59298536b27e95096148b1bb12f80fc4

commit 82d9545a59298536b27e95096148b1bb12f80fc4
Author: jdoerrie <jdoerrie@chromium.org>
Date: Tue Apr 17 13:57:48 2018

[base] Add missing methods to base::span

This change implements constructors for base::span that allow the
construction from a pair of pointers, std::arrays and containers
supporting base::data and base::size, e.g. std::initializer_list.
In addition, this change adds operator().

Bug:  788913 
Change-Id: Ibc280eef1c7e47a5a27e92503dda3614ef5513a4
Reviewed-on: https://chromium-review.googlesource.com/981139
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551313}
[modify] https://crrev.com/82d9545a59298536b27e95096148b1bb12f80fc4/base/containers/span.h
[modify] https://crrev.com/82d9545a59298536b27e95096148b1bb12f80fc4/base/containers/span_unittest.cc
[modify] https://crrev.com/82d9545a59298536b27e95096148b1bb12f80fc4/base/containers/span_unittest.nc

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c5ca9f13ffa33418061a326e178384d6461aa713

commit c5ca9f13ffa33418061a326e178384d6461aa713
Author: Mohsen Izadi <mohsen@chromium.org>
Date: Tue Apr 17 14:44:43 2018

Revert "[base] Add missing methods to base::span"

This reverts commit 82d9545a59298536b27e95096148b1bb12f80fc4.

Reason for revert: compile failures on ios-simulator-xcode-clang and ios-device-xcode-clang builders:
 * https://uberchromegw.corp.google.com/i/chromium.mac/builders/ios-simulator-xcode-clang/builds/46343
 * https://uberchromegw.corp.google.com/i/chromium.mac/builders/ios-device-xcode-clang/builds/57515

Original change's description:
> [base] Add missing methods to base::span
> 
> This change implements constructors for base::span that allow the
> construction from a pair of pointers, std::arrays and containers
> supporting base::data and base::size, e.g. std::initializer_list.
> In addition, this change adds operator().
> 
> Bug:  788913 
> Change-Id: Ibc280eef1c7e47a5a27e92503dda3614ef5513a4
> Reviewed-on: https://chromium-review.googlesource.com/981139
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551313}

TBR=dcheng@chromium.org,jdoerrie@chromium.org

Change-Id: Ia967227caa3a24a10df2ab4c5fa4ee03dddf0f29
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  788913 
Reviewed-on: https://chromium-review.googlesource.com/1014761
Reviewed-by: Mohsen Izadi <mohsen@chromium.org>
Commit-Queue: Mohsen Izadi <mohsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551329}
[modify] https://crrev.com/c5ca9f13ffa33418061a326e178384d6461aa713/base/containers/span.h
[modify] https://crrev.com/c5ca9f13ffa33418061a326e178384d6461aa713/base/containers/span_unittest.cc
[modify] https://crrev.com/c5ca9f13ffa33418061a326e178384d6461aa713/base/containers/span_unittest.nc

Status: WontFix (was: Started)
While r551313 now allows construction a base::span<const T> from a std::initializer_list<T> via the container constructor [1], this unfortunately does not solve the issue raised in #c1, as the template deduction fails here. What is now possible is to simplify the workaround from 

void Foo(std::initializer_list<int> list) {
  Foo(base::make_span(list.begin(), list.end()));
}

to

void Foo(std::initializer_list<int> list) {
  Foo(base::make_span(list));
}

Smarter deduction will require a dedicated constructor overload taking a std::initializer_list, similarly to how Abseil does it [2]. However, since this constructor is not part of the official standard proposal, we shouldn't add it at this point. Marking at WontFix again.

[1] http://eel.is/c++draft/span.cons#itemdecl:5
[2] https://github.com/abseil/abseil-cpp/blob/a7e522daf1ec9cda69b356472f662142dd0c1215/absl/types/span.h#L342-L344
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/11201f072ca67ad3046b0720fa89840f4fd62981

commit 11201f072ca67ad3046b0720fa89840f4fd62981
Author: Jan Wilken Doerrie <jdoerrie@chromium.org>
Date: Wed Apr 18 11:03:58 2018

Reland "[base] Add missing methods to base::span"

This is a reland of 82d9545a59298536b27e95096148b1bb12f80fc4

Original change's description:
> [base] Add missing methods to base::span
>
> This change implements constructors for base::span that allow the
> construction from a pair of pointers, std::arrays and containers
> supporting base::data and base::size, e.g. std::initializer_list.
> In addition, this change adds operator().
>
> Bug:  788913 
> Change-Id: Ibc280eef1c7e47a5a27e92503dda3614ef5513a4
> Reviewed-on: https://chromium-review.googlesource.com/981139
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551313}

TBR=dcheng@chromium.org

Bug:  788913 
Change-Id: I2e02879ddd2839ce0c26c8b18e565d555e08cc7b
Reviewed-on: https://chromium-review.googlesource.com/1013468
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551627}
[modify] https://crrev.com/11201f072ca67ad3046b0720fa89840f4fd62981/base/containers/span.h
[modify] https://crrev.com/11201f072ca67ad3046b0720fa89840f4fd62981/base/containers/span_unittest.cc
[modify] https://crrev.com/11201f072ca67ad3046b0720fa89840f4fd62981/base/containers/span_unittest.nc

Sign in to add a comment