ykjit / yk

yk packages
https://ykjit.github.io/yk/
Other
28 stars 7 forks source link

Split `InstIdx` iteration into two. #1252

Closed ltratt closed 1 month ago

ltratt commented 1 month ago

While doing work elsewhere, I realised that we need to iterate over instructions in two ways:

  1. Only iterate over "instructions that do something" (i.e. hide Proxy* and Tombstone).
  2. Iterate over all instructions (including Proxy* and Tombstone).

However, our existing iter_inst_idx only did (1) -- and it's easy to misread it and assume it will do (2).

This commit makes iter_inst_idx do (2), with iter_skipping_inst_idx do (1). I did consider getting rid of the latter, but that just forces more knowledge of Proxy* and Tombstone around the system in a way that doesn't seem very helpf way that doesn't seem very helpful.

vext01 commented 1 month ago

This superficially looks OK, although I wasn't around for the discussion on proxy instructions, so I may lack context.

My only nit is that iter_skipping_inst_idx makes it sound like we are iterating something but skipping the inst_idx component of it. But I think the name really means "iterate inst_idxs, but skip some of them", right?

I wonder if a boolean flag to iter_inst_idx (whether to skip or not) would be clearer?

ltratt commented 1 month ago

In a sense skipping_iter is more accurate, but I think people assume iterator methods to start with iter?

I wonder if a boolean flag to iter_inst_idx (whether to skip or not) would be clearer?

No, because the return types are different.

vext01 commented 1 month ago

I risk bikeshedding, so I suggest we merge and refine if it doesn't feel good?

ltratt commented 1 month ago

Agreed: we can always rename later.