[PATCH] D50340: [Local] Add dbg location on unreachable inst in changeToUnreachable

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: unittests/Transforms/Utils/Local.cpp:616
+  BasicBlock &BB = F.front();
+  Instruction *A = BB.getFirstNonPHI();
+
----------------
Why not just `.front()`?


================
Comment at: unittests/Transforms/Utils/Local.cpp:620
+  // One instruction should be affected.
+  EXPECT_EQ(changeToUnreachable(A, false), (unsigned)1);
+
----------------
nit, `1U` is easier to read.


================
Comment at: unittests/Transforms/Utils/Local.cpp:620
+  // One instruction should be affected.
+  EXPECT_EQ(changeToUnreachable(A, false), (unsigned)1);
+
----------------
vsk wrote:
> nit, `1U` is easier to read.
Please add a comment when using boolean literals as parameters. Instead of 'false', it's clearer to write `/*UseLLVMTrap*/false`.


================
Comment at: unittests/Transforms/Utils/Local.cpp:626
+  ASSERT_TRUE(isa<UnreachableInst>(B));
+  ASSERT_TRUE(B->hasMetadata());
+}
----------------
This needs to check that the correct location is present, not just that the instruction has metadata.


Repository:
  rL LLVM

https://reviews.llvm.org/D50340