microsoft / GSL

Guidelines Support Library
Other
6.18k stars 740 forks source link

array_view should have a constructor that takes an initializer_list #137

Closed mattnewport closed 9 years ago

mattnewport commented 9 years ago

I started trying to convert some code that provides safety / convenience wrappers around D3D API calls over to use gsl::array_view and ran into a problem that means I can't support the interface I want. I have calling code that looks like this:

// defined somewhere 
Microsoft::WRL::ComPtr<ID3D11DeviceContext> context;
Micrsofot::WRL::ComPtr<ID3D11Buffer> vertices;
UINT stride;

// calling code somewhere else
IASetVertexBuffers(context, 0, {vertices.Get()}, {stride});

And a wrapper around ID3D11DeviceContext::IASetVertexBuffers() with signature:

void IASetVertexBuffers(
  [in]                 UINT                StartSlot,
  [in]                 UINT                NumBuffers,
  [in, optional]       ID3D11Buffer *const *ppVertexBuffers,
  [in, optional] const UINT                *pStrides,
  [in, optional] const UINT                *pOffsets
);

That currently looks like this:

template <typename Context, typename Buffers = std::initializer_list<ID3D11Buffer*>,
          typename Strides = std::initializer_list<UINT>>
void IASetVertexBuffers(const Context& context, unsigned startSlot, const Buffers& buffers,
                        const Strides& strides) {
    UINT offsets[D3D11_IA_VERTEX_INPUT_RESOURCE_SLOT_COUNT] = {};
    context->IASetVertexBuffers(UINT(startSlot), UINT(std::size(buffers)), std::begin(buffers),
                                std::begin(strides), std::begin(offsets));
};

This wrapper lets me directly pass a braced init list as an argument for buffers and strides (the default std::initializer_list template arguments allow the compiler to deduce the type correctly in this case) or any other Container type that supports begin() and end().

I'd like to convert the buffers and strides parameters to gsl::array_views over the appropriate types. Currently however gsl::array_view has no constructor taking an std::initializer_list which means I can't get my desired call-site syntax of directly passing a braced init list and having a gsl::array_view implicitly constructed from it. I could write my own as_array_view helper but I hate unnecessary typing at the call site :-)

I think a gsl::array_view constructor taking an std::initializer_list would give me the call syntax I'd like but I may be missing some subtlety that means that wouldn't work. Still, one way or another I'd like to be able to directly pass braced init lists to functions taking a gsl::array_view of the corresponding type.

mattnewport commented 9 years ago

So as an experiment I added an array_view constructor like this:

// from std::initializer_list
constexpr array_view(std::initializer_list<value_type> il) : array_view(std::begin(il), std::size(il))
{
}

And it seems to give me what I want. There may be hidden problems I'm not seeing here yet but I'm going to start using this and see if I run into any...

Horstling commented 9 years ago

The standard states that

An object of type std::initializer_list is constructed from an initializer list as if the implementation allocated a temporary array of N elements of type const E [...] The array has the same lifetime as any other temporary object

So if I understand this correctly, a array_view to a initializer list could/should become a dangling reference right after your constructor has finished. Keep in mind that all the standard containers are taking copies of the initializer list`s elments.

EDIT: I guess in your use case you are copying the elments somewhere down the call tree within the expression that created the initializer list, so you are fine. But having such a constructor makes it very easy to write code like:

array_view<int> arr  = { 1, 2, 3 };
// arr is a dangling reference now
mattnewport commented 9 years ago

The standard also says:

Temporary objects are destroyed as the last step in evaluating the full-expression (1.9) that (lexically) contains the point where they were created.

This is why you can safely write code like:

f(g(a));

or

for (auto x : {1, 2, 3}) std::cout << x << ", ";

At least I hope I understand that correctly, otherwise an awful lot of my code is broken :-)

Your edit concern is no different than any other use of array_view:

array_view<int> arr = vector<int>{1, 2, 3};
// arr is a dangling reference now

vector<int> f();
array_view<int> arr = f();
// arr is a dangling reference now

This is C++, the fact that you can write broken code should not prevent you from creating a reasonable interface to your class.

// These are fine from a lifetime point of view and should all compile
void g(array_view<int> av);
g(vector<int>{1, 2, 3});
g(f());
g({1, 2, 3});

If code needs access to the elements of an array_view beyond the point of return of the function to which it is an argument then of course it needs to copy it. Again, this is no different than for an array_view over any other container.

Horstling commented 9 years ago

I see your point, but in my personal opinion a constructor taking a initializer list would just make it too easy to write innocent looking but broken code.

On the other hand, if array_view is primarly a vehicle for passing stuff into a function, so that most array_view objects are themselves temporaries (which is in fact the case I guess), then you are right.

gdr-at-ms commented 9 years ago

The suggested constructor increases convenience at the expense of safety.

mattnewport commented 9 years ago

I strongly disagree with this. How is this any less safe than an implicit constructor from any other container? Why would you allow the equally dangerous auto av = array_view<int>{vector<int>{1, 2, 3}; but not auto av = array_view<int>{{1, 2, 3}};?

This is not just about convenience but about expressivity. This may seem minor but it's actually a big enough issue that I'd consider writing my own array_view alternative to get this syntax.

If you're worried about the safety issue then why is the constructor from a container not explicit? Presumably for convenience at the expense of safety?

gdr-at-ms commented 9 years ago

A good point that it would make sense to add array_view constructors taking && and deleted (e.g. "=delete"). That should be a separate request.

mattnewport commented 9 years ago

@gdr-at-ms If the array_view constructors taking && are deleted then is there any objection to having a constructor that takes a const std::initializer_list& so this compiles:

auto il = {1, 2, 3, 4};
auto av = array_view<const int>{il};

This is less useful than if the && constructors are deleted but still handy to have.

neilmacintosh commented 9 years ago

I actually have concerns about an initializer-list constructor. It gives the impression that an array_view is a container (it is not). You should initialize an array_view from a container - the thing it is a view over - or as a copy/restriction of another array_view. I do not think an initializer_list makes a better container than an array or std::array for cases like the snippet you gave.

mattnewport commented 9 years ago

@neilmacintosh I prefer initializer_lists over plain arrays or std::array in this case because they are less verbose and less redundant. I don't have to specify the type anywhere the way I do with both a plain array and an std::array. In addition, std::array also forces you to redundantly specify the element count:

int cArr[] = {1, 2, 3}; // bad, redundant int
auto stdArr = std::array<int, 3>{1, 2, 3}; // double bad, redundant int and element count
auto il = {1, 2, 3}; // better
f({1, 2, 3}); // best

There should probably be a standard std::make_array() helper to improve the situation with std::array but that's a separate issue.

gdr-at-ms commented 9 years ago

@mattnewport Something is unclear in your use case scenario. Why can't you write

std::array<int, 3> arr { 1, 2, 3 };

in the first place? Granted your example of stdArr is needlessly suspicious, but there is nothing that would force you to write that code in the first place.

Regarding

auto il = { 1, 2, 3 };

I am sure you are aware that requires the compiler to create the exact moral equivalent of std::array<int, 3>, then build on top of that another 2-word structure (the initializer list). If you want to construct an array_view on top of that, you would construct another 2-word structure on top of the 2-word structure on top of the std::array<int, 3>.
It sounds to me as if what is brandished as "best" is the result of pursuing syntax "nit" at the expense of semantics, and safety. I suspect you must have more convincing real world use case than this...

mattnewport commented 9 years ago

@gdr-at-ms I'm not sure which part of my use case is unclear so I'll try and explain more fully.

The motivating example from real code wrapping real Microsoft APIs is included in the original post at top of thread. In that particular case I'm trying to (thinly) wrap APIs that take pointers to arrays of COM pointers and a size (and sometimes additional pointers to equal sized arrays of UINTs and possibly other types) and make them more convenient and safer to use. In this case, more convenient means minimally verbose and safer means not redundantly specifying a size. There's an additional wrinkle in that in many places I have WRL::ComPtrs and not raw COM interface pointers so in the common case of a one element array I can't simply pass 1, &&ptr as code that works with raw COM interface pointers rather than a smart pointer wrapper typically would.

The wrapper lets calling code go from:

// defined elsewhere
ID3D11DeviceContextPtr context; // All typedefs ending in Ptr are WRL::ComPtrs
ID3D11BufferPtr vertexBuffer;
struct Vertex;
// Old calling code
ID3D11Buffer* buffers[] = { vertexBuffer.Get() };
UINT strides[] = { UINT(sizeof(Vertex)) }; // strides and offsets size must match buffers size
UINT offsets[] = { UINT(0) };
context->IASetVertexBuffers(0, UINT(std::size(buffers)), buffers, strides, offsets);

to:

// Wrapped calling code
IASetVertexBuffers(context, 0, {vertexBuffer.Get()}, {UINT(sizeof(Vertex))}, {UINT(0)});

Setting up to draw a particular scene element requires calls to multiple similar APIs and gets quite verbose. When iterating during development, it is not uncommon to change the number of COM objects passed in some of these arrays (adding extra vertex buffers, textures, etc.) and it is easy to forget to update a size somewhere for a matching array. It's very common to encounter code in a large code base where the buffer count is hard coded too and not deduced from the array size. The wrapper can check the sizes of all arrays match as a side benefit (the real code does, I omitted it here because it was not the point of the example).

I claim that the 'after' syntax above is preferable to defining separate named arrays for a number of reasons. First, it does not introduce unnecessary named objects into the scope. Second, it limits the lifetime of those objects to the minimal scope in which they are required without introducing extra curlies. Third it avoids redundantly naming types or specifying lengths that can be deduced directly.

The original wrapper used template arguments so it can accept std::initializer_list, std::array, a C array or a std::vector. This is desirable because in different contexts, any one of those may make more sense. I don't want to default to vector and introduce unnecessary dynamic allocation but sometimes I will have a vector already and want to pass it directly. I don't want to default to std::array because sometimes I will have a dynamically sized container. I don't want to radically change the API because that's potentially a lot of work and requires people already familiar with D3D to learn a different set of functions and types.

I seem to believe rather more strongly than you that syntax is important and concise code that specifies exactly what is required is a desirable goal. Being able to provide an anonymous initalizer_list directly when that exactly matches the intent is a big win in code clarity IMO. Any potential overhead here is negligible - even if the compiler isn't smart enough to optimize it away the underlying API calls are doing significantly more work than copying a few pointer sized objects around on the stack so it won't register. This is a very different scenario than bounds checks in a tight loop for example where any overhead really does significantly impact performance.

gdr-at-ms commented 9 years ago

I saw the example. Yes, it is written with std::initializer_list. But is that what it should be? Why aren't strides and offsets of type std::array? The comment says that strides and offsets must match buffers in size, but the code does not say that (only the comment does). I suspect once the code is brought into line with the comment, the "need" for initializer_list will start weakening...

mattnewport commented 9 years ago

@gdr-at-ms I hit 'Comment' by accident before I finished writing the post, hopefully the finished post answers your questions more fully.

gdr-at-ms commented 9 years ago

Yes, we're aware of existing code base that hardcode array bounds and forget to keep things in sync. @neilmacintosh has "tied array views" to catch the fast majority of such cases that we've seen in real shipping products. When the code was rewritten in terms of array views and tied array views, not only was it more correct, but it was shorter and more direct in expressing the intent.

Regardig syntax: yes, all of us involved in the Core Guidelines and its support library (GSL) appreciate the importance of syntax (check Bjarne's D&E). The way I put it is: good notation reflects good semantics.

mattnewport commented 9 years ago

@gdr-at-ms I think with the GSL you appreciate the value of incrementally converting existing code to be safer without wholesale rewrites. In this case, that's exactly what the wrapper code is intended to do: incrementally convert existing code calling D3D APIs to be a little safer and clearer. Part of that goal means wrappers that 'mechanically' convert to the original APIs and don't try and redesign them. In an ideal world existing code could be converted using an automatic tool, in practice it is a matter of some combination of regex search and replace and manual tweaking. For that reason, a 'better designed' API is not on the cards, just a wrapping of {size, pointer} args with something like an array_view.

Given that, and the fact that existing calling code may use any combination of vector, C array, std::array, pointer and size or even initializer_list as a source of data, it is desirable that a simple wrapper can handle all of those. Given that some functions take more than one pointer argument like in the example, overloading on each argument type is not desirable due to the combinatorial explosion.

Under these constraints, I think directly passing an std::initializer_list in the common but by no means universal situation that the element count is fixed but there is no pre-existing array-like container of them to hand at the call site is the best syntax. It is minimally verbose, safe and directly expresses the intent of the calling code without polluting the local scope with named temporaries.

Tied array views sound an like interesting improvement. Are they already in the GSL?

gdr-at-ms commented 9 years ago

We fully appreciate incremental adoption. At the same time, we are very mindful of not encouraging practices that leave partially converted codes in states that are arguably worse than what they were, or "transitional" practices that create more holes. I have no doubt that a competent, well-trained, seasoned, professional C++ programmer would know how to use std::initializer_list with array_view properly without creating more vulnerability holes -- just like we have no doubt that many of the constructs we either ban or strongly discourage in the C++ Core Guidelines can be used very well by a ninja dev.

I suspect someone needing badly this std::initialiazer_list convenience can add that as a non-member helper function. That will not only emphasize the transitional nature of the facility, but also call out that it isn't part of the interface of array_view.

When Bjarne and I introduced std::initializer_list, it was (and is) a device for bridging the gap between a "literal" notation and a container construction. There is no container construction in the example you show. Rather, it looks more like a reroute of that helper type for purposes other than constructing a container. While it is a perfectly legal code, it nevertheless should look suspicious to a trained eye.

mattnewport commented 9 years ago

@gdr-at-ms I don't know the details of the intent behind initializer_list but my use seems no more suspicious than this common idiom:

for (const auto x : { 1,2,3,5,8,13,21,34 }) cout << x << '\n';

And fundamentally at both a syntactic and semantic level my use is identical to the use in a constructor argument: it is a parameter to a function call that has a lifetime guaranteed to encompass the scope of the function body and it provides a contiguous array of elements. The exact same attributes that make it a great language feature for initializing a container (minimal syntactic overhead due to automatic type and size deduction based from the list elements) make it ideal for my use case.

gdr-at-ms commented 9 years ago

The usage in the for-loop is very different: the lifetime of the object is readily apparent and available. I don't see that it is identical usage when all you have is a function signature. See the rules by Herb and Neil about how they infer object lifetimes.

gdr-at-ms commented 9 years ago

When you construct a container from an std::initializer_list, the container constructor can assume at most that the initializer list is alive only during the construction of the container -- so it copies the elements. Similarly, in the loop above, the loop (body) makes the assumption that the initializer list is alive (at most) only during the loop body. None of that assumption is apparent in any form if you were to make an array view out of an initializer list.

mattnewport commented 9 years ago

@gdr-at-ms I don't follow. How are these meaningfully different:

// direct use
for (const auto x : { 1,2,3,5,8,13,21,34 }) cout << x << '\n';
// via a constructor
struct Foo { 
    Foo(std::initializer_list<int> il) {
        for (const auto x : il) cout << x << '\n';
    }
}
auto f = Foo{ 1,2,3,5,8,13,21,34 };
// via a function call
void f(std::initializer_list<int> il) {
    for (const auto x : il) cout << x << '\n';
}
f({ 1,2,3,5,8,13,21,34 });
// via a hypothetical implicitly initialized array_view
void g(std::array_view<const int> av) {
    for (const auto x : av) cout << x << '\n';
}
g({ 1,2,3,5,8,13,21,34 });

Taking an array_view argument is a clear statement that you do not own the lifetime of the backing storage and that therefore you will not attempt to access it outside the scope of the function. That semantics perfectly matches taking an array_view wrapped around a temporary initializer_list as an argument. A function that takes an array_view knows that its argument must be assumed out of scope once the function body exits.

Note that I don't really care to be able to do this:

auto il = { 1,2,3,5,8,13,21,34 };
g(il);

That's merely a fallback request given the denial of the request to allow this:

g({ 1,2,3,5,8,13,21,34 });

And just makes it easier for me to use a wrapper like this everywhere:

template<typename C>
array_view<std::add_const_t<typename C::value_type>> av(const C& x) { return {x}; }
g(av({ 1,2,3,5,8,13,21,34 }));

To work around a flawed array_view interface that deletes && constructors.

gdr-at-ms commented 9 years ago

You asked for intent; this document is still good: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2215.pdf

We may disagree on interface design and intended semantics of a class; I very much want us to keep discussion civil.

mattnewport commented 9 years ago

@gdr-at-ms I apologize if I was un-civil. I think this issue is important hence the somewhat impassioned debate and I do believe an array_view with deleted && constructors is flawed for the reasons I'm attempting to explain but I don't intend to be overly combative.

I must profess that I don't understand your concerns here though. I really don't see the difference between the cases outlined above in terms of semantics or safety. I don't really think the original intent behind the design of initializer_list is particularly relevant here. Many features of C++ have been used beyond the original intent behind their design, not least templates. If the feature fits an originally unforeseen use case that is not a bad thing. I believe Bjarne has said as much himself when discussing the history of C++.

gdr-at-ms commented 9 years ago

The concern is very simple: constructing an array_view from an expiring value is a active potential memory and bound safety hole. The fact that a ninja dev can avoid that trap does not make it less of a trap. We designed array_view to help promote and guarantee memory and bound safety. We don't view an interface that saves a reference to known temporary objects as appropriate.

As you can see, we are back to what array_view is designed to accomplish. The GSL types are designed (and implemented) to serve the Core Guidelines goals. If they can serve more, without compromising what they were designed for, that is great. If you disagree with GSL's goals, then more often than not you will be disagreeing passionately with GSL types :-)

mattnewport commented 9 years ago

@gdr-at-ms http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2215.pdf seems to provide some support for my position that it is an appropriate argument to a non constructor function to me (emphasis mine):

4.4 Syntax

In the EWG there were strong support for the idea of the sequence constructor, but initially no consensus about the syntax needed to express it. There was a strong preference for syntax to make the “special” nature of a sequence constructor explicit. ... Based on extensive discussions, we prefer the X(initializer_list) design, because this “special compiler-recognized class name” approach ...

  • The initializer_list type can be used for any argument that can accept an initializer list. For example int f(int, initializer_list, int) can accept calls such as f(1, {2,3}, 4). This eliminates the need for variable argument lists (… arguments) in many (most?) places.
mattnewport commented 9 years ago

@gdr-at-ms

The concern is very simple: constructing an array_view from an expiring value is a active potential memory and bound safety hole. The fact that a ninja dev can avoid that trap does not make it less of a trap.

I really think this is throwing the baby out with the bathwater. You don't have to be a 'ninja dev' to safely use an array_view like this:

vector<int> f();
void g(array_view<const int>);
g(f());
g({1, 2, 3});

Now if you want to go out of your way to shoot yourself in the foot and do this:

auto av = array_view{f()};
g(av);
auto av = array_view{{1, 2, 3}};
g(av);

Then that's unfortunate but the first use case is the much more natural and likely one and prohibiting it for the sake of avoiding the latter errors seems massive overkill. Instead leverage the static analysis tools that go along with GSL to catch the latter. That also gives you the ability to catch other wilfully perverse uses like:

auto a = make_unique<int[]>({1, 2, 3});
auto av = array_view{a.get(), 3};
a.reset();
g(av);

And all the other myriad ways you can shoot yourself in the foot with lifetimes in C++ that simply eliminating && constructors doesn't help you at all with.

gdr-at-ms commented 9 years ago

I could not catch what you wanted to show by the two posts that were mostly quotes. Could you elaborate?

mattnewport commented 9 years ago

Apologies, the formatting was broken when I clicked comment. If you look at them now on the issue page they should be legible.

On Sun, Oct 11, 2015, 20:13 Gabriel Dos Reis notifications@github.com wrote:

I could not catch what you wanted to show by the two posts that were mostly quotes. Could you elaborate?

— Reply to this email directly or view it on GitHub https://github.com/Microsoft/GSL/issues/137#issuecomment-147281148.

gdr-at-ms commented 9 years ago

Ah, things were displayed on my side as quotes.

Regarding the quote of section 4.4 in your previous comment (how does one get a link to a specific comment?), I don't see how it supports your ask: the quote there was about allowing general usage. The issue here is whether array_view should be designed to allow construction from an initializer list. The answer is no. Neither pushes the other in any specific direction.

gdr-at-ms commented 9 years ago

Regarding the other comment. Some people want the ability to have potentially unsafe interfaces and then rely on tools to catch them; others (including the authors of the Core Guidelines and GSL) want people to write safe interfaces by default, then use tools assistance when they stray away.

We are back to the goals of the Core Guidelines and the GSL.

mattnewport commented 9 years ago

@gdr-at-ms

Regarding the quote of section 4.4 in your previous comment (how does one get a link to a specific comment?), I don't see how it supports your ask: the quote there was about allowing general usage.

Perhaps I was misunderstanding what you were saying earlier, but you appeared to be objecting to the usage of an std::initializer_list argument in any other context than constructing a container. I gave examples in an earlier comment (I haven't figured out how to link back to a specific comment either) of usage as an argument to regular function calls and argued that that was just as valid a use. I see the quote from section 4.4 as supporting the use of std::initializer_list as an argument to a regular function. So my claim is that these are all safe, valid and good uses of initializer lists:

void f(std::initializer_list<int>);
template<typename Container> // Should really be more constrained but for simplicity
void g(const Container&);
f({1, 2, 3});
g({1, 2, 3});
for (auto x : {1, 2, 3}) { ... }

Do you disagree with any of those uses? Now I'd also claim that this is safe, valid and good:

vector<int> h();
g(h());

And I assume that's not controversial. So the issue at hand is around this:

int controversial(array_view<const int>);
auto i = controversial({1, 2, 3});
auto i = controversial(h());
struct Foo { Foo() : i{controversial(h())} {}; int i; };

I think we agree this is safe, we're debating whether it should be valid and we seem to disagree over whether it's good. Currently I don't see any way at the library level to allow this without also allowing:

auto badAv = array_view<const int>{1, 2, 3};
auto badAv = array_view<const int>{h()};

Which we both agree is unsafe, are debating whether it should be valid and agree that it is not good. The fundamental discussion then is whether allowing the first to compile is a good enough reason to allow the second to compile, or conversely whether preventing the second from compiling is important enough to disallow the first. I assume if there was a library-level way to allow the first and not the second then it wouldn't really be an issue for debate?

Some people want the ability to have potentially unsafe interfaces and then rely on tools to catch them; others (including the authors of the Core Guidelines and GSL) want people to write safe interfaces by default, then use tools assistance when they stray away.

I would argue that array_view is inherently an unsafe interface. A non-owning view in C++ cannot be made safe at the library level and can only perhaps be made safe through tooling. I'm by no means a Rust expert but Rust seems to be trying to take a language-level approach to disallowing such unsafety but nothing about GSL or array_view at the library level can make it safe.

The current discussion seems to me to be about applying a library level tool to make one particular unsafe usage non-compiling while doing nothing at the library level to address the many other ways of forming an unsafe array_view - quite reasonably since C++ currently does not provide the language level facilities to make something like array_view safe in all cases. Any hope of doing so must come from tooling. In the process many safe, valid and syntactically desirable uses of array_view are prohibited for little practical advantage.

I say little practical advantage because the unsafe uses highlighted above seem both unlikely to occur in practice and relatively easy for static analysis tools to catch relative to all the other ways to use an array_view unsafely.

Now the reason I took an interest in GSL's array_view is because I was looking for the convenience benefits of the older array_view proposal and that led me to the newer array_view proposal from GSL. I'm not going to say no to the promise of additional safety on top of the convenience benefits and static safety provided by the old array_view but I am going to look elsewhere if convenience is overly hampered by ineffective library level attempts to gain a little safety. I am also going to look elsewhere if performance is severely negatively impacted by runtime bounds checking but that's a whole separate discussion.

I'm broadly supportive of the goals of GSL but I believe adoption of safer coding practices is far more likely if the safer way also ends up being a more convenient, elegant and cleaner way. The great thing about much of modern C++ is that you can largely have your cake and eat it - get better safety, equally good or even better performance and more concise, elegant and simpler code. I'd hate to see an array_view that bucks that trend and makes for less elegant code with more redundancy in a (misguided in my view) attempt to close some safety loopholes with language features that are not suited to the task.

mattnewport commented 9 years ago

@gdr-at-ms @neilmacintosh I think it's worth highlighting again that a major reason I feel so strongly about this issue is that it currently appears that gsl::array_view will ultimately become std::array_view. It's currently unclear to me to what extent that is the case however. I have no strong desire to try and push GSL to change its goals and aims but I do have a very strong interest in the form of any std::array_view. If it turns out that the two are not in fact coupled then I am happy to direct my suggestions purely to the array_view proposed for standardization.

gdr-at-ms commented 9 years ago

Two things: (a) I was explaining why we designed std::initializer_list in the first place; then make a remark that 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. I stand by both remarks.

(b) If you believe that array_view is inherently unsafe, please to let us know of the cause of such belief so we can improve the interface.

mattnewport commented 9 years ago

@gdr-at-ms a) We do disagree on this point then - I strongly believe that there is nothing suspicious about:

for (auto x : { 1, 2, 3 }) { ... }

Or any of the other similar examples I posted. Indeed, I think these are good modern C++ style. This can be nothing but a purely stylistic concern on your part since the semantics are unambiguously safe?

b) I didn't think I was making a controversial statement here. I merely meant that there is nothing at a language or library level that can make a non-owning array_view safe in C++ from such things as:

auto v = vector<int>{1, 2, 3};
auto av = array_view<int>{v};
v.push_back(4);

Or any one of the other countless ways you could break an array_view in C++. This is the kind of problem that better static analysis tools and more sophisticated compiler diagnostics can hope to tackle but that can't be addressed by language / library level tricks like deleting a useful constructor.

gdr-at-ms commented 9 years ago

@mattnewport I am afraid you missed the point :-(. As I explained several comments back when you brought up the loop example the first time. Furthermore, read carefully what I wrote; you will notice that the loop example does not use std::initializer_list. If it does, it would look suspicious to a trained eye, but it doesn't.

mattnewport commented 9 years ago

@gdr-at-ms I'm very keen to understand your point which still eludes me I'm afraid, hence my efforts to clarify exactly what you are saying. You said (emphasis mine):

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. I stand by both remarks.

I'm trying to understand in what sense the following do not "use" std::initializer_list in a context other than constructing a container:

for (auto x : { 1, 2, 3 }) { ... }
void f(initializer_list<int>);
f({1, 2, 3});

So neither of those are 'uses' by your definition? I'm afraid you're making some subtle distinction on the meaning of 'use' that you'll have to clarify further for me.

gdr-at-ms commented 9 years ago

In the for-loop, there is no std::initializer_list. There is an implicit construction of container In the declaration of f(), there is one. There is no container construction In the call to f(), there is none. There is an implicit construction of a container.

From the three, one should be suspicious -- the middle one. Hopefully, that is clear.

mattnewport commented 9 years ago

I'm afraid I can't parse what you wrote there. You're saying it's the declaration of f() that's suspicious? That's the usage that is explicitly endorsed in the quote from the initializer list paper I brought up previously though.

On Mon, Oct 12, 2015, 09:36 Gabriel Dos Reis notifications@github.com wrote:

In the for-loop, there is no std::initializer_list. There is an implicit construction of container In the declaration of f(), there is one. There is no container construction In the call to f(), there is none. There is an implicit construction of a container.

From the three, one should be suspicious -- the middle one. Hopefully, that is clear.

— Reply to this email directly or view it on GitHub https://github.com/Microsoft/GSL/issues/137#issuecomment-147454627.

gdr-at-ms commented 9 years ago

I am afraid we have reached an infinite loop where each side claims it does not understand (I certainly do not understand your last comment). I will leave it there.

mattnewport commented 9 years ago

@gdr-at-ms I'm not trying to be obtuse but I really can't make sense of what you wrote here, please help me out by explaining it in slightly clearer terms. I do want to understand your position.

In the for-loop, there is no std::initializer_list. There is an implicit construction of container In the declaration of f(), there is one. There is no container construction In the call to f(), there is none. There is an implicit construction of a container.

neilmacintosh commented 9 years ago

This conversation is now off-topic and making no progress. It has veered onto a general discussion of initializer_list. While that's interesting, the original issue of "should array_view have a constructor that takes initializer_list" has already been resolved as declined.

mattnewport commented 9 years ago

@neilmacintosh Lost in the discussion above was a question about the degree to which GSL's array_view design is coupled to any proposal for an std::array_view. I'd still like to understand the answer to that so as to know where to focus further discussion. I'm not overly concerned about the design of GSL array_view in isolation but am concerned to the degree it influences any std::array_view.

neilmacintosh commented 9 years ago

@mattnewport Sure, it's a perfectly reasonable question.

I am proposing to have the GSL array_view standardized. The initial version of the proposal is available here: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0122r0.pdf. I am about to issue a revision that tightens it up significantly from that first, somewhat rushed version.

So you were definitely following the right path by raising your concerns here first...as influencing the GSL array_view is the simplest path to influence the proposal. I can see that you feel the interface of GSL array_view doesn't meet your needs. However, I am not willing to adopt constructors for either initializer_list or Container&&, as they would conflict with the design of the type and broader aims of the GSL.

If you remain concerned about what a std::array_view would look like, then LEWG is the right place to raise your concerns. What does/does not end up getting standardized from my proposal is largely up to that working group.

mnaydenov commented 7 years ago

I am not sure if this thread is still valid, but I wanted to vote for span/array_view being able to be created from a initializer_list. My reasons are: