[PATCH] D69999: [DebugInfo] Support for DW_OP_implicit_pointer (IR Verifier and Bitcode)

[Alok Kumar Sharma via Phabricator via llvm-commits]

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

In D69999#1758776 <https://reviews.llvm.org/D69999#1758776>, @Orlando wrote:

> Hi @alok,
>
> Adrian said:
>
> > Thanks! Let's cycle back to this after the llvm-dev thread that @dblaikie started has been resolved.
>
> Reading the thread I'm not sure if it counts as having been 'resolved', so hopefully these comments are not out of place. 
>  To help keep things running smoothly please make sure:
>  a) variable names are capitalised, and
>  b) comments are written as sentences (capitals, punctuation),


Thanks for your comments. I will update it in next version.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:77
+  // debugging information entry that describes the dereferenced object’s value.
+  // when this operation is created, the debugging information entry is not yet
+  // formed. To solve this problem, we maintain a vector of dereferenced
----------------
Orlando wrote:
> when -> When
Thanks for your comment. It will be updated in next version.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:79-80
+  // formed. To solve this problem, we maintain a vector of dereferenced
+  // objects. We keep index of dereferenced object's at first operand of
+  // DW_OP_implicit_pointer operation temporarily. This temporary value is
+  // later replaced by actual value when available.
----------------
Orlando wrote:
> I get what this is saying but I found it a little hard to parse, and as Adrian pointed out elsewhere 'later' is a bit vague:
> 
> `We keep index of dereferenced object's at first operand of DW_OP_implicit_pointer operation temporarily. This temporary value is later replaced by actual value when available.`
Thanks for your comment. It will be updated in next version.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:82
+  // later replaced by actual value when available.
+  std::vector<const DINode *> ImplicitVars;
+
----------------
Orlando wrote:
> Could `std::vector` be `llvm::SmallVector` [0] here?
> 
> [0] http://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h
> 
Thanks for your comment. It will be updated in next version.


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

https://reviews.llvm.org/D69999