On Wed, Sep 9, 2026 at 3:49 PM Richard Sandiford <[email protected]> wrote: > > 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?
I think this is a sound approach, not too far off existing practice in GCC. It sounds like this could be done separately, weeding out the save/restore of recog_data_d. An alternative to the macro would be making it a singleton instance of a class that converts to recog_data but allows the "flip" via some push/pop but internally manages a stack of pointers to recog_data. > 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
