Matt Newport
2015-10-17 22:42:43 UTC
I raised an issue <https://github.com/Microsoft/GSL/issues/137> against GSL
array_view recently (which is forming the basis for a propsal for an
std::array_view) which led to a side discussion about when it is
appropriate to use std::initializer_list and braced init lists. In that
discussion Gaby Dos Reis said "any use of std::initializer_list in context
other than constructing a container, in particular an array_view should be
suspicious to a trained eye.". The issue comments weren't deemed the right
place to continue discussion of that topic but I'm still left confused
about why it should be. It doesn't seem to be true to me but if there's a
good reason I'm not seeing I'd like to understand it.
I'm going to be pasting a bunch of code snippets below, a live version of
the full example they come from can be found here
<http://coliru.stacked-crooked.com/a/a897b1943902419a>.
So as of C++11 all the following range based for loops work and as far as I
am aware are all perfectly safe:
constexpr int intArray[] = {1, 2, 3};
const auto intStdArray = array<int, 3>{4, 5, 6};
const auto intVector = vector<int>{7, 8, 9};
const auto intList = list<int>{10, 11, 12};
const auto intInitList = {13, 14, 15};
auto makeVector = [] { return vector<int>{16, 17, 18}; };
for (auto i : intArray) cout << i << ", ";
cout << '\n';
for (auto i : intStdArray) cout << i << ", ";
cout << '\n';
for (auto i : intVector) cout << i << ", ";
cout << '\n';
for (auto i : intInitList) cout << i << ", ";
cout << '\n';
for (auto i : makeVector()) cout << i << ", ";
cout << '\n';
for (auto i : vector<int>{19, 20, 21}) cout << i << ", ";
cout << '\n';
for (auto i : {21, 22, 23}) cout << i << ", ";
cout << '\n';
The version that constructs an explicit temporary vector<int>{19, 20, 21}
is less efficient and more verbose than the version that uses a braced init
list {21, 22, 23} directly. In the case that you know the items you want to
iterate over but don't have an existing container to hand the direct braced
init list seems to me the most desirable version of the code.
Now say I want to factor this code out into a function. Perhaps my first
attempt might be:
void printIntsVector(const vector<int>& v) {
for (auto i : v) cout << i << ", ";
cout << '\n';
}
printIntsVector(intVector);
printIntsVector(intInitList);
printIntsVector(makeVector());
printIntsVector(vector<int>{19, 20, 21});
printIntsVector({21, 22, 23}); // Implicitly creates a temporary vector
and allocates memory
// doesn't compile
// printIntsVector(intArray);
// printIntsVector(intStdArray);
// printIntsVector(intList);
This isn't a great solution since it doesn't handle all the containers I
might want to use and in the case that I pass a braced init list directly
it implicitly constructs a temporary vector which likely allocates memory.
A second attempt might be:
template <typename Cont = std::initializer_list<int>>
void printIntsContainer1(const Cont& c) {
for (auto i : c) cout << i << ", ";
cout << '\n';
}
printIntsContainer1(intArray);
printIntsContainer1(intStdArray);
printIntsContainer1(intVector);
printIntsContainer1(intList);
printIntsContainer1(intInitList);
printIntsContainer1(makeVector());
printIntsContainer1(vector<int>{19, 20, 21});
printIntsContainer1({21, 22, 23});
This is better in my eyes: it works with all the containers that worked
before factoring the code out into a function and in the case of directly
passing a braced init list it doesn't create a temporary vector. If I
understand Gaby's position correctly though, this code should be considered
suspicious since it uses an std::initializer_list in a context other than
constructing a container. If this is actually to be considered bad style
I'd like to understand why.
Perhaps we want to generalize this code further though and not restrict it
to containers of ints. One possible way to achieve that is:
template <typename Cont>
void printContainer2(const Cont& c) {
printContainer1(c);
}
template <typename T>
void printContainer2(std::initializer_list<T> il) {
printContainer1(il);
}
This still works with all the previous cases but now additionally allows
code like this to compile:
printContainer2(vector<const char*>{"one", "two", "three"});
printContainer2({"one", "two", "three"});
Which is nice to have. Now this version seems to be crying out to take some
kind of generic Range argument but hopefully that's something we'll get
down the line with Eric Niebler's Range work. Again, this seems to be
considered bad style by Gaby for reasons that are not clear to me.
Now we get closer to the original motivating example that caused me to
raise the array_view issue: interoperating with legacy APIs that expect
contiguous arrays of elements. Say I'd like to be able to call a function
like bsearch with any container guaranteeing contiguous storage. It seems
to me like it would be good to have an array_view like class that would
help with this:
template <typename T>
class ArrayView {
public:
ArrayView(initializer_list<T> il) : first{std::begin(il)},
last{std::end(il)} {}
template <typename Cont>
ArrayView(Cont& c) : first{std::data(c)}, last{first + std::size(c)} {}
auto begin() const { return first; }
auto end() const { return last; }
auto size() const { return last - first; }
private:
const T* first;
const T* last;
};
void printIntArrayView(ArrayView<int> av) {
for (auto e : av) cout << e << ", ";
cout << '\n';
}
void wrapLegacyApi(ArrayView<int> av, int searchVal) {
auto comp = [](const void* x, const void* y) {
auto toInt = [](const void* x) {
int res;
memcpy(&res, x, sizeof(res));
return res;
};
return toInt(x) < toInt(y) ? -1 : toInt(x) == toInt(y) ? 0 : 1;
};
const auto findIt = reinterpret_cast<const int*>(
bsearch(&searchVal, begin(av), size(av), sizeof(*begin(av)), comp));
if (findIt) cout << "Found " << searchVal << " at position " << (findIt
- begin(av)) << '\n';
else cout << "Did not find " << searchVal << '\n';
}
wrapLegacyApi(intArray, 2);
wrapLegacyApi(intStdArray, 2);
wrapLegacyApi(intVector, 7);
// These are all forbidden by GSL array_view
wrapLegacyApi(intInitList, 14);
wrapLegacyApi(makeVector(), 18);
wrapLegacyApi(vector<int>{19, 20, 21}, 21);
wrapLegacyApi({21, 22, 23}, 22);
This is where GSL array_view takes a different view and forbids
construction of a GSL array_view from a temporary (by deleting the
constructors that take a Container&&) or from an std::initializer_list
(even a non-temporary). I understand the reasoning behind that decision
since broken code like this would compile:
auto badAv1 = array_view<const int>{{1, 2, 3}};
// dangling reference to temporary initializer list
auto badAv2 = array_view<const int>{makeVector()};
// dangling reference to temporary vector
But I don't see how that makes it suspicious to use temporary initializer
lists in general. I'm interested to know if there are hidden downsides of
using std::initializer_list in contexts other than the argument to a
container constructor that I'm not seeing, or if there are other reasons
any of the above should be considered bad modern C++ style that I'm not
aware of.
Thanks,
Matt.
array_view recently (which is forming the basis for a propsal for an
std::array_view) which led to a side discussion about when it is
appropriate to use std::initializer_list and braced init lists. In that
discussion Gaby Dos Reis said "any use of std::initializer_list in context
other than constructing a container, in particular an array_view should be
suspicious to a trained eye.". The issue comments weren't deemed the right
place to continue discussion of that topic but I'm still left confused
about why it should be. It doesn't seem to be true to me but if there's a
good reason I'm not seeing I'd like to understand it.
I'm going to be pasting a bunch of code snippets below, a live version of
the full example they come from can be found here
<http://coliru.stacked-crooked.com/a/a897b1943902419a>.
So as of C++11 all the following range based for loops work and as far as I
am aware are all perfectly safe:
constexpr int intArray[] = {1, 2, 3};
const auto intStdArray = array<int, 3>{4, 5, 6};
const auto intVector = vector<int>{7, 8, 9};
const auto intList = list<int>{10, 11, 12};
const auto intInitList = {13, 14, 15};
auto makeVector = [] { return vector<int>{16, 17, 18}; };
for (auto i : intArray) cout << i << ", ";
cout << '\n';
for (auto i : intStdArray) cout << i << ", ";
cout << '\n';
for (auto i : intVector) cout << i << ", ";
cout << '\n';
for (auto i : intInitList) cout << i << ", ";
cout << '\n';
for (auto i : makeVector()) cout << i << ", ";
cout << '\n';
for (auto i : vector<int>{19, 20, 21}) cout << i << ", ";
cout << '\n';
for (auto i : {21, 22, 23}) cout << i << ", ";
cout << '\n';
The version that constructs an explicit temporary vector<int>{19, 20, 21}
is less efficient and more verbose than the version that uses a braced init
list {21, 22, 23} directly. In the case that you know the items you want to
iterate over but don't have an existing container to hand the direct braced
init list seems to me the most desirable version of the code.
Now say I want to factor this code out into a function. Perhaps my first
attempt might be:
void printIntsVector(const vector<int>& v) {
for (auto i : v) cout << i << ", ";
cout << '\n';
}
printIntsVector(intVector);
printIntsVector(intInitList);
printIntsVector(makeVector());
printIntsVector(vector<int>{19, 20, 21});
printIntsVector({21, 22, 23}); // Implicitly creates a temporary vector
and allocates memory
// doesn't compile
// printIntsVector(intArray);
// printIntsVector(intStdArray);
// printIntsVector(intList);
This isn't a great solution since it doesn't handle all the containers I
might want to use and in the case that I pass a braced init list directly
it implicitly constructs a temporary vector which likely allocates memory.
A second attempt might be:
template <typename Cont = std::initializer_list<int>>
void printIntsContainer1(const Cont& c) {
for (auto i : c) cout << i << ", ";
cout << '\n';
}
printIntsContainer1(intArray);
printIntsContainer1(intStdArray);
printIntsContainer1(intVector);
printIntsContainer1(intList);
printIntsContainer1(intInitList);
printIntsContainer1(makeVector());
printIntsContainer1(vector<int>{19, 20, 21});
printIntsContainer1({21, 22, 23});
This is better in my eyes: it works with all the containers that worked
before factoring the code out into a function and in the case of directly
passing a braced init list it doesn't create a temporary vector. If I
understand Gaby's position correctly though, this code should be considered
suspicious since it uses an std::initializer_list in a context other than
constructing a container. If this is actually to be considered bad style
I'd like to understand why.
Perhaps we want to generalize this code further though and not restrict it
to containers of ints. One possible way to achieve that is:
template <typename Cont>
void printContainer2(const Cont& c) {
printContainer1(c);
}
template <typename T>
void printContainer2(std::initializer_list<T> il) {
printContainer1(il);
}
This still works with all the previous cases but now additionally allows
code like this to compile:
printContainer2(vector<const char*>{"one", "two", "three"});
printContainer2({"one", "two", "three"});
Which is nice to have. Now this version seems to be crying out to take some
kind of generic Range argument but hopefully that's something we'll get
down the line with Eric Niebler's Range work. Again, this seems to be
considered bad style by Gaby for reasons that are not clear to me.
Now we get closer to the original motivating example that caused me to
raise the array_view issue: interoperating with legacy APIs that expect
contiguous arrays of elements. Say I'd like to be able to call a function
like bsearch with any container guaranteeing contiguous storage. It seems
to me like it would be good to have an array_view like class that would
help with this:
template <typename T>
class ArrayView {
public:
ArrayView(initializer_list<T> il) : first{std::begin(il)},
last{std::end(il)} {}
template <typename Cont>
ArrayView(Cont& c) : first{std::data(c)}, last{first + std::size(c)} {}
auto begin() const { return first; }
auto end() const { return last; }
auto size() const { return last - first; }
private:
const T* first;
const T* last;
};
void printIntArrayView(ArrayView<int> av) {
for (auto e : av) cout << e << ", ";
cout << '\n';
}
void wrapLegacyApi(ArrayView<int> av, int searchVal) {
auto comp = [](const void* x, const void* y) {
auto toInt = [](const void* x) {
int res;
memcpy(&res, x, sizeof(res));
return res;
};
return toInt(x) < toInt(y) ? -1 : toInt(x) == toInt(y) ? 0 : 1;
};
const auto findIt = reinterpret_cast<const int*>(
bsearch(&searchVal, begin(av), size(av), sizeof(*begin(av)), comp));
if (findIt) cout << "Found " << searchVal << " at position " << (findIt
- begin(av)) << '\n';
else cout << "Did not find " << searchVal << '\n';
}
wrapLegacyApi(intArray, 2);
wrapLegacyApi(intStdArray, 2);
wrapLegacyApi(intVector, 7);
// These are all forbidden by GSL array_view
wrapLegacyApi(intInitList, 14);
wrapLegacyApi(makeVector(), 18);
wrapLegacyApi(vector<int>{19, 20, 21}, 21);
wrapLegacyApi({21, 22, 23}, 22);
This is where GSL array_view takes a different view and forbids
construction of a GSL array_view from a temporary (by deleting the
constructors that take a Container&&) or from an std::initializer_list
(even a non-temporary). I understand the reasoning behind that decision
since broken code like this would compile:
auto badAv1 = array_view<const int>{{1, 2, 3}};
// dangling reference to temporary initializer list
auto badAv2 = array_view<const int>{makeVector()};
// dangling reference to temporary vector
But I don't see how that makes it suspicious to use temporary initializer
lists in general. I'm interested to know if there are hidden downsides of
using std::initializer_list in contexts other than the argument to a
container constructor that I'm not seeing, or if there are other reasons
any of the above should be considered bad modern C++ style that I'm not
aware of.
Thanks,
Matt.
--
---
You received this message because you are subscribed to the Google Groups "ISO C++ Standard - Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to std-discussion+***@isocpp.org.
To post to this group, send email to std-***@isocpp.org.
Visit this group at http://groups.google.com/a/isocpp.org/group/std-discussion/.
---
You received this message because you are subscribed to the Google Groups "ISO C++ Standard - Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to std-discussion+***@isocpp.org.
To post to this group, send email to std-***@isocpp.org.
Visit this group at http://groups.google.com/a/isocpp.org/group/std-discussion/.