[PATCH] D153990: [DebugInfo][RemoveDIs] Add prototype storage classes for non-instruction variable debug-info

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

LGTM + a few comments

> Finally, I've noticed that this patch won't build independently as there are some forward declarations of stuff that doesn't have a definition, that later needs a definition -- this is due to the shifting sands of this patch series. I could jam in various shims to make it work; or I could just land the next patch at the same time, which works fine. I think that's a reasonable approach as these were only split up to make review easier anyway.

If the two commits can't be easily separated, perhaps better to squash them? YMMV.



================
Comment at: llvm/include/llvm/IR/DebugProgramInstruction.h:368
+  static iterator_range<simple_ilist<DPValue>::iterator> getEmptyDPValueRange(){
+    return make_range(EmptyDPMarker.StoredDPValues.begin(), EmptyDPMarker.StoredDPValues.end());
+  }
----------------
> This will bite us if someone tries to insert a DPValue in that range

Is there no way to create an empty range without an object to grab iterators from?

If not, you could return `make_range(EmptyDPMarker.StoredDPValues.end(), EmptyDPMarker.StoredDPValues.end());` (end(), end()) to avoid those issues? wdyt?







================
Comment at: llvm/lib/IR/LLVMContextImpl.cpp:50
+  // Check that any variable location records that fell off the end of a block
+  // when it's terminator was removed, were eventually replaced. This assertion
+  // firing indicates that DPValues went missing during the lifetime of the
----------------
nit: remove comma


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:1661-1663
+    if (!TrailingDPValues.count(B))
+      return nullptr;
+    return TrailingDPValues[B];
----------------
`DenseMap::lookup` returns a default constructed ValueType (so, `nullptr` here).


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

https://reviews.llvm.org/D153990