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.
jeff
