[PATCH] D134817: Remove the dependency between lib/DebugInfoDWARF and MC

[Adrian Prantl via Phabricator via llvm-commits]

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

I think this is looking good now. I have a few very minor comments, but otherwise this is good to go.



================
Comment at: lldb/source/Expression/DWARFExpression.cpp:73
+
+  std::function<llvm::Optional<llvm::StringRef>(uint64_t DwarfRegNum,
+                                                bool isEH)>
----------------
rastogishubham wrote:
> aprantl wrote:
> > Since a `StringRef` can be `empty()`, it usually doesn't make a lot of sense to wrap it inside of `Optional`. I would just use a plain `StringRef`.
> The reason I used an Optional is because in my mind the RegName for a register can be empty itself, maybe? The Optional tells me that there was no RegName or no LLVMRegNum found and helps me output any error messages if needed.
I guess that's technically correct, but it would be weird. Then we would print an empty string instead of the DWARF register number, which means there would be no record of that there was a register? I would probably just treat an empty string like a nullptr.


================
Comment at: llvm/include/llvm/DebugInfo/DIContext.h:204
+      GetNameForDWARFReg = nullptr;
+  bool isEH = false;
 
----------------
clayborg wrote:
> Case should be "IsEH" according to llvm guidelines.
Can you swap the order between the fields? The bools may have a smaller alignment than the std::function and so the whole struct could be smaller if they are grouped together.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:1256
                            Optional<uint64_t> Offset) const {
+  DumpOpts.isEH = IsEH;
   if (Offset) {
----------------
I think the suggestion to pass IsEH in DumpOpts was well-intentioned, but it's not so elegant that we need to update it here. I don;t feel strongly enough about it to be bothered by it though.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:248
+        Opcode == DW_OP_bregx)
+      OS << format(" %s%+" PRId64, RegName->data(), Operands[OpNum]);
+    else
----------------
Wouldn't it be more efficient to say 

```
OS << '  ' << RegName << format("%+" PRId64, Operands[OpNum]);
```
?


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

https://reviews.llvm.org/D134817