On Fri, May 16, 2025 at 1:05 AM Jeff Law <jeffreya...@gmail.com> wrote: > > > > On 5/15/25 11:18 AM, Richard Sandiford wrote: > > The start_sequence/end_sequence interface was a big improvement over > > the previous state, but one slightly awkward thing about it is that > > you have to call get_insns before end_sequence in order to get the > > insn sequence itself: > I can't even remember what the previous state was... push_to_sequence? > > > > > To get the contents of the sequence just made, you must call > > `get_insns' *before* calling here. > > > > We therefore have quite a lot of code like this: > > > > insns = get_insns (); > > end_sequence (); > > return insns; > > > > It would seem simpler to write: > > > > return end_sequence (); > > > > instead. > Totally agreed. It's much more natural to just return the sequence. > > > > > I can see three main potential objections to this: > > > > (1) It isn't obvious whether ending the sequence would return the first > > or the last instruction. But although some code reads *both* the > > first and the last instruction, I can't think of a specific case > > where code would want *only* the last instruction. All the emit > > functions take the first instruction rather than the last. > Understood. It's a bad name. finish_sequence, close_sequence, or > somesuch would probably be much clearer. And no, I'm not immediately > aware of cases where we'd only look at the last instruction. I could > probably come up with one in theory, but again, not aware of any case in > practice. > > > > > (2) The "end" in end_sequence might imply the C++ meaning of an exclusive > > endpoint iterator. But for an insn sequence, the exclusive endpoint > > is always the null pointer, so it would never need to be returned. > > That said, we could rename the function to something like > > "finish_sequence" or "complete_sequence" if this is an issue. > See above. It's a large mechanical change, but it would remove some of > the ambiguity in the current API. > > > > (3) There might have been an intention that start_sequence/end_sequence > > could in future reclaim memory for unwanted sequences, and so an > > explicit get_insns was used to indicate that the caller does want > > the sequence. > > > > But that sort of memory reclaimation has never been added, > > and now that the codebase is C++, it would be easier to handle > > using RAII. I think reclaiming memory would be difficult to do in > > any case, since some code records the individual instructions that > > they emit, rather than using get_insns. > IMHO this isn't a strong reason to keep the status quo. And as you > note, if we really wanted to wade into this space, RAII style > ctors/dtors would likely be a much better way to go. > > I'm absolutely in favor of the change, even if we didn't do a name > change in the process. But let's give others a bit of time to chime in.
I agree with the change, thus OK. Richard. > > jeff >
