[PATCH] D134817: Remove the dependency between lib/DebugInfoDWARF and MC
Adrian Prantl via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 28 11:02:53 PDT 2022
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
More information about the llvm-commits
mailing list