[PATCH] D70260: [DebugInfo] Support for DW_OP_implicit_pointer (salvagDebugInfo improvement).

[Alok Kumar Sharma via Phabricator via llvm-commits]

alok marked 5 inline comments as done.
alok added a comment.

In D70260#1758219 <https://reviews.llvm.org/D70260#1758219>, @Orlando wrote:

> Hi @alok. I haven't been following the DW_OP_implicit_pointer discussion too closely, so just some inline nitpick comments/questions from me.
>
> Thanks,
> Orlando


Thanks for your comments. Next version would have all your comments incorporated. Please let me know if you have any more comments.



================
Comment at: llvm/include/llvm/IR/Module.h:197
+  /// we will preserve location of variable in std::map to use later
+  std::map<Value *, Value *> VarAddrMap;
 
----------------
Orlando wrote:
> Are you using a `std::map` for a specific reason? Could you not use a `std::unordered_map`, or better yet, a `DenseMap` [0] or similar llvm container here instead?
> 
> [0] http://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h
Thanks for the suggestion, It will be updated in next version to use DenseMap.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1000-1001
 bool DIExpression::extractIfOffset(int64_t &Offset) const {
-  if (getNumElements() == 0) {
+  unsigned numElements;
+  numElements = getNumElements();
+
----------------
Orlando wrote:
> Why not `unsigned numElements = getNumElements();` ?
Thanks for your comment. It will be updated in next version.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:1642-1643
   auto wrapMD = [&](Value *V) { return wrapValueInMetadata(Ctx, V); };
+  bool changed = false;
+  Value *debugDeclareAddress = nullptr;
+
----------------
Orlando wrote:
> Please can you capitalise these variable names?
Thanks for your comment. It will be updated in next version.


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

https://reviews.llvm.org/D70260