On Tue, May 12, 2015 at 5:00 PM, [email protected] <[email protected]> wrote:
> In http://reviews.llvm.org/D9714#171426, @hfinkel wrote: > > > In http://reviews.llvm.org/D9714#171408, @rsmith wrote: > > > > > 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. > > > > > > Will do. > > > > > With that said, I'm going to review this as if for upstream clang. > > > > > > Thanks! (The point of doing this is to get feedback on > implementation-related issues) > > > > > 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. > > > > > > > > > > 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. > > > > > > That makes sense (and this is very similar to how we currently handle > std::initializer_list). > > > > Any opinion on: > > > > 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). > 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. > 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). > It all depends on your design. If you expect this to work: int a[n]; // "secretly" has type std::arb<int> std::arb<int> b = a; // non-owning handle to 'a' then std::arb should not destroy anything. 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? 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: std::arb<int> a = {(auto int[n]){}, n}; // aggregate initialization, even if arb<int> is not an aggregate, n evaluated only once 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. > 2. The automatically-included header (or similar) with a simpler class, > or just requiring the header and making the class more fully-featured? > We require a header to be included for std::initializer_list and typeinfo; this seems similar.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
