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

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a subscriber: clayborg.
aprantl added a comment.

I think this is a good change to make, I just have a couple of comments inline about the exact implementation.



================
Comment at: lldb/source/Expression/DWARFExpression.cpp:73
+
+  std::function<llvm::Optional<llvm::StringRef>(uint64_t DwarfRegNum,
+                                                bool isEH)>
----------------
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`.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:124
 
+  std::function<Optional<StringRef>(uint64_t RegNum, bool isEH)> Callback =
+      nullptr;
----------------
Callback isn't a very descriptive name: What about `GetNameForDWARFReg` or something like that?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:167
+            std::function<Optional<StringRef>(uint64_t RegNum, bool isEH)>
+                Callback = nullptr) const;
 
----------------
I am mildly surprised that `= nullptr` works. Should the default argument be a lambda or static function that returns `llvm::None`?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:226
+  void dump(raw_ostream &OS, bool IsEH,
+            std::function<Optional<StringRef>(uint64_t RegNum, bool isEH)>
+                Callback = nullptr) const;
----------------
This type appears a lot of times. What about declaring it with a `using` directive or making it a class object instead?


================
Comment at: llvm/lib/DebugInfo/DWARF/CMakeLists.txt:39
   Object
-  MC
   Support
   )
----------------
Very nice!


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:338
     } else {
-      Loc.dumpRange(Offset, EndOffset - Offset, OS, MRI, Obj, DumpOpts);
+      Loc.dumpRange(Offset, EndOffset - Offset, OS, Obj, DumpOpts, Callback);
     }
----------------
Would there any benefit of having DWARFContext own Callback instead of passing it through here everywhere?


================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:287
     return createStringError(std::errc::invalid_argument,
                              "unable to create DWARF context");
 
----------------
@clayborg Would this be a regression? It seems to not be tested...


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

https://reviews.llvm.org/D134817