[PATCH] D138977: [IR][NFC] Adds BasicBlock::splice().

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added inline comments.


================
Comment at: llvm/include/llvm/IR/BasicBlock.h:466
+    auto FromItLast = std::next(FromIt);
+    if (ToIt == FromIt || ToIt == FromItLast)
+      return;
----------------
aeubanks wrote:
> vporpo wrote:
> > aeubanks wrote:
> > > aeubanks wrote:
> > > > should add a test for this, e.g. `splice(I1, I2)` where `I1` is right before `I2` in the same block
> > > and does the actual implementation not handle this?
> > I am not sure. Since I am basically calling splice() internally I was relying on it being well tested. But anyway, since this is part of the BB API now, I agree that it needs a test.
> if `getInstList().splice()` already handles this, then I'd say no need for this early return. would be good to double check
I just checked and it won't handle this, but I don't think we can handle this either. The current behavior is that it just crashes when the iterator goes out of bounds as it never finds the End iterator. I added a test that confirms the crash.

I don't think we can handle this any better. We could add a linear-time check to test whether BeginIt is before EndIt, but I am not sure it is worth it. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138977