Richard Biener <[email protected]> writes: > On Tue, Sep 8, 2026 at 11:15 PM feedable <[email protected]> wrote: >> >> >> On 08/09/2026 11:34, Richard Biener wrote: >> > On Sat, Sep 5, 2026 at 7:39 PM feedable <[email protected]> wrote: >> >> >> >> On 25/08/2026 13:48, Richard Biener wrote: >> >>> On Tue, Aug 25, 2026 at 1:26 AM feedable via Sourceware Forge >> >>> <[email protected]> wrote: >> >>>> From: feedable <[email protected]> >> >>>> >> >>>> Currently, auto_vec defaults its copy&move operations. This is bad, >> >>>> since a user that tries to copy or move an auto_vec will unknowingly >> >>>> set himself up for trouble, since the pointers from the base and >> >>>> members >> >>>> will be copied verbatim, leading to double free situtaions and >> >>>> the like. >> >>> For auto_vec<T, N> with N != 0 we do not want these, instead they >> >>> should >> >>> be deleted. As seen in your implementation the complexity >> >>> involved with >> >>> having to deal with the stack backing for small number of elements >> >>> makes >> >>> this very inefficient. If copy/move is needed people should use >> >>> auto_vec<T, 0> >> >>> instead. >> >> As stated in the discussion for v1, a move constructor is required for a >> >> value to be returned. Values of type auto_vec<T, N> with N != 0 are >> >> returned in the codebase already (yes, this is currently UB if the >> >> constructor is not elided). >> > IMO those uses should be fixed. >> I'm sorry but I still fail to understand the purpose of forbidding those >> operations, given that an implementation of these operations is present >> in this patch. >> >> It is fairly clear to me that forbidding these operations will incur >> costs on the codebase, such as denying storage of `auto_vec`s in >> containers (which is being used in the following patch), and denying the >> ability to return them (which is used now in the code elsewhere). >> >> What is not clear to me is the benefits of such a decision. >> >> Regarding the efficiency of my implementation of those ops, claims about >> that can only be made in comparison, and the alternative here is to >> unconditionally allocate on the heap, which is worse than storing the >> data inline and eliding the move on return. > > I'm not sure it is worse. > >> Until the net-benefits are clear to me, I cannot in good faith move >> forward with this change. > > The argument is that once you expose an inefficient API you'll > get lazy uses and inefficiencies that could have been easily avoided. > But I might be overly pessimistic here, not knowing how often the > copy/move construction/assignment ops for auto_vec would occur. > > There is also the semantic difference with vec<> where > assignment does a shallow copy of the m_vec pointer and _not_ > copy the data as with your non-move assignment/ctor. So can > we at least avoid that? vec<> has vec<>::copy for this and > truncate() + splice() for the assignment. > > We should possibly delete vec<>::operator=() and adjust code > to use const vec<>& argument passing (it's one more indirection > sadly). Adding move assignment might be useful, but I'd like > to explicitly "see" it, like if we can force v1 = std::move (v2); or so?
AIUI, the use case for this change is to allow copying of recog_data. But IMO that isn't a good motivation. I mentioned in a previous review that: I suppose the underlying problem is that recog_data shouldn't be a global variable. Copying and restoring it is a hack to get around that design decision. And that hack was borderline acceptable while the copy was simple. IMO the ideal fix would be to remove the global state and handle the cache in a different way. But that would be way too invasive. Even changing recog_data to a pointer would probably be too much churn (although I wouldn't be against it personally). So perhaps we should adopt a hack used elsewhere and make recog_data a macro that [deferences a] pointer (either global, or in crtl or something). By default the pointer points at a global recog_data_d, but backends can use an RAII class or direct pointer assignment to temporarily switch it to a temporary recog_data_d instead. It's all a bit icky, but maybe more acceptable than copying auto_vecs? AFAIK, no-one responded to that, so I don't know whether it sounds like a bad idea. :) But my point is that the motivating case seems to be precisely the kind of lazy use that you're worried about. I.e. we have an unfortunate design decision (a global recog_data object, despite now being effectively stacked state) and we have worked around that by copying a large structure wholesale. The copy will become even more expensive once operands[] is no longer a fixed size. Thanks, Richard
