<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, May 12, 2015 at 5:00 PM, <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">In <a href="http://reviews.llvm.org/D9714#171426" target="_blank">http://reviews.llvm.org/D9714#171426</a>, @hfinkel wrote:<br>
<br>
> In <a href="http://reviews.llvm.org/D9714#171408" target="_blank">http://reviews.llvm.org/D9714#171408</a>, @rsmith wrote:<br>
><br>
> > First off: I'm not happy about having this extension in upstream clang until we have a strong indication that this is the direction that will end up standardized. For now, I'd recommend maintaining this as a clang fork on github or similar.<br>
><br>
><br>
> Will do.<br>
><br>
> > With that said, I'm going to review this as if for upstream clang.<br>
><br>
><br>
> Thanks! (The point of doing this is to get feedback on implementation-related issues)<br>
><br>
> > I don't like that you create two variables here. We try to maintain as much source fidelity as we can, and I think we can do better here -- how about instead introducing a new form of expression that represents "stack-allocate a certain amount of memory" (with a subexpression for the initialization, if you allow these variables to have an initializer), much like MaterializeTemporaryExpr does for a SD_Automatic temporary, but parameterized by an expression specifying the array size? Then create just a single expression of the std::arb type, initialized by that expression.<br>
><br>
> ><br>
><br>
> > You should also introduce a Type subclass to represent type sugar for the ARB type, so that we can model that int[n] desugars to std::arb<int> but should be pretty-printed as an array type.<br>
><br>
><br>
> That makes sense (and this is very similar to how we currently handle std::initializer_list).<br>
><br>
> Any opinion on:<br>
><br>
> 1. Should we bother with the access-control-overriding init type? We could just make the constructor public but make it UB if the user calls it (it would be implementation-specific anyhow).<br></span></blockquote><div><br></div><div>My first inclination would be to model the initialization of std::arb exactly like the initialization of std::initializer_list. (In fact, the two types seem to be identical except that std::initializer_list holds a const pointer whereas std::arb would hold a non-const pointer.) That is: directly initialize the members of the aggregate, don't call a constructor.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> 2. Should I make std::arb manage the lifetime of the objects directly? If it just takes a special allocation expression maybe that's more natural? I'd like to not force extra work for POD types (but I imagine I could use some enable_if/is_pod logic to leave PODs uninitialized).<br></span></blockquote><div><br></div><div>It all depends on your design. If you expect this to work:</div><div><br></div><div>    int a[n]; // "secretly" has type std::arb<int></div><div>    std::arb<int> b = a; // non-owning handle to 'a'</div><div><br></div><div>then std::arb should not destroy anything.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
</span>Or to put it another way, should the aforementioned new "stack-allocate a certain amount of memory" expression also be responsible for calling the constructors of non-POD array elements and register calling their destructors as cleanup, or should that logic be embedded in std::arb?</blockquote><div><br></div><div>Unless you have a reason to do otherwise, I'd put the responsibility for registering a cleanup with the expression that performs the initialization. The model I'm imagining is that the initialization is implicitly doing this:</div><div><br></div><div>  std::arb<int> a = {(auto int[n]){}, n}; // aggregate initialization, even if arb<int> is not an aggregate, n evaluated only once</div><div><br></div><div>where (auto int[n]{}) is the creation of an automatic storage duration VLA temporary. With that perspective, the destruction belongs with the creation of the temporary.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">> 2. The automatically-included header (or similar) with a simpler class, or just requiring the header and making the class more fully-featured?<br></div></div></blockquote><div><br></div><div>We require a header to be included for std::initializer_list and typeinfo; this seems similar.</div></div></div></div>