labath added a comment.
I didn't go into all the details of this patch, as I am guessing this will
still go through a number of revisions, but I did make a couple notes of things
that caught my eye while glancing over it.
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointInjectedSite.h:50
+class BreakpointInjectedSite
+ : public std::enable_shared_from_this<BreakpointInjectedSite>,
+ public BreakpointSite {
----------------
BreakpointSite is already shared_ptr-enabled. Why this? I don't think that's
how you're supposed to use this class.
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointSite.h:54-56
+ static bool classof(const BreakpointSite *bp_site) {
+ return bp_site->getKind() == eKindBreakpointSite;
+ }
----------------
This is weird. So, in OO terminology, `BreakpointInjectedSite` "is-a"
`BreakpointSite`, but if you ask `llvm::isa<BreakpointSite>(injected_site)`, it
will return false?
================
Comment at: lldb/include/lldb/Expression/ExpressionVariable.h:210
+public:
+ typedef AdaptedIterable<collection, lldb::ExpressionVariableSP,
+ vector_adapter>
----------------
What is the purpose of this `AdaptedIterable`, and why is it better than just
returning say `ArrayRef<lldb::ExpressionVariableSP` ?
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:226
+ m_trap_addr = addr;
+ LLDB_LOGV(log, "Injected trap address: {:llx}", addr);
+ return true;
----------------
This isn't a proper format string. You can find their general description here
<http://llvm.org/docs/ProgrammersManual.html#formatting-strings-the-formatv-function>,
and the specifics are generally next to the definition of the actual format
provider (e.g.
<https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Support/FormatProviders.h#L102>).
I find it very helpful to look at their unit tests too:
<https://github.com/llvm-mirror/llvm/blob/master/unittests/Support/FormatVariadicTest.cpp>.
================
Comment at: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.h:96-98
+ uint64_t GetJumpOpcode() override { return X64_JMP_OPCODE; }
+
+ size_t GetJumpSize() override { return X64_JMP_SIZE; }
----------------
replace with `ArrayRef<uint8_t> GetJumpOpcode();` or something similar.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66249/new/
https://reviews.llvm.org/D66249
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
