[PATCH] D152655: [MC] Merge MC[Sub,Super]RegIterator with mc_[sub,super]_reg_iterator

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 15 14:19:49 PDT 2023


dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/MC/MCRegisterInfo.h:511
+  // Cache the current value, so that we can return a reference to it.
+  MCPhysReg Val;
+
----------------
barannikov88 wrote:
> barannikov88 wrote:
> > @dblaikie Could you comment on this?
> > This requires type type to be default constructible. In D152098 I add MCRegUnit that does not have a good default value. Maybe I should instead return a proxy object from `operator*` that will hold the value? Would that be correct w.r.t. iterator requirements? Probably factor the proxy out into a common (templated) base class, too.
> > 
> Ah I guess this won't change a thing, I would still need to be able to construct the proxy from some value, which I don't have in case of the end iterator.
> 
If you need something that isn't default constructible/can't just have an instance hanging around as a member like this, the next best thing would be `std::optional<T>` so it can be non-constructed. If the overhead of the extra bool/padding in `std::optional` is significant, you could maybe use a raw aligned buffer and /maybe/ conditionalize on the validity of the underlying iterator to decide whether there's an object in that buffer or not. But ensuring the code doesn't leak/fail to construct/destroy properly would be subtle.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152655/new/

https://reviews.llvm.org/D152655



More information about the llvm-commits mailing list