[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:
> 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.


================
Comment at: llvm/unittests/IR/BasicBlockTest.cpp:386
+
+TEST(BasicBlockTest, SpliceFromFirstLast) {
+  LLVMContext Ctx;
----------------
aeubanks wrote:
> this name is confusing, not sure what it's referring to
Yeah good point. 
I think I will also rename the arguments of `BasicBlock::splice` because `Last` actually refers to the iterator after tha last one. So `End` is a better name for it.


================
Comment at: llvm/unittests/IR/BasicBlockTest.cpp:415
+  Instruction *ToRet = &*ToBBIt++;
+  ToBB->splice(ToI1->getIterator(), FromBB, FromI2->getIterator(),
+               FromBr->getIterator());
----------------
aeubanks wrote:
> perhaps also test when we splice more than one instruction this way? e.g. `FromI1` instead
Agreed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138977