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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/include/llvm/IR/BasicBlock.h:466
+    auto FromItLast = std::next(FromIt);
+    if (ToIt == FromIt || ToIt == FromItLast)
+      return;
----------------
vporpo wrote:
> 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?
I think adding an assert, even if it's linear time, would be good to help future users. can add it under expensive checks (and gate the death test too) if it's too expensive in assert builds


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138977