mib added inline comments.
================
Comment at: lldb/include/lldb/Breakpoint/BreakpointSite.h:54-56
+ static bool classof(const BreakpointSite *bp_site) {
+ return bp_site->getKind() == eKindBreakpointSite;
+ }
----------------
labath wrote:
> This is weird. So, in OO terminology, `BreakpointInjectedSite` "is-a"
> `BreakpointSite`, but if you ask `llvm::isa<BreakpointSite>(injected_site)`,
> it will return false?
No, it returns **true**.
================
Comment at: lldb/include/lldb/Expression/ExpressionVariable.h:210
+public:
+ typedef AdaptedIterable<collection, lldb::ExpressionVariableSP,
+ vector_adapter>
----------------
labath wrote:
> What is the purpose of this `AdaptedIterable`, and why is it better than just
> returning say `ArrayRef<lldb::ExpressionVariableSP` ?
Given a class with a container, `AdaptedIterable` will generate the proper
iterator accessor for this class.
In this case, it's pretty convenient to do:
```
for (auto var : expr_vars)
{ }
```
instead of:
```
for (auto var : expr_vars.GetContainer())
{ }
```
It doesn't require to have a getter on the container to iterate over it.
Most of LLDB container classes are built upon `std` containers (vector, map
...).
I could change it to `ArrayRef<lldb::ExpressionVariableSP>`, but I don't see
why using it would be better, + it might break things (e.g. ArrayRef doesn't
implement some of the `std` containers method like `erase()` or `clear()`).
================
Comment at:
lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:181
+ # FCB falls back to regular conditional breakpoint that get hit once.
+ self.assertTrue(breakpoint.GetHitCount() == 1)
----------------
aprantl wrote:
> how does this test ensure that the breakpoint was actually injected and that
> it didn't fall back to a regular breakpoint?
The breakpoint doesn't get injected if we fallback to a regular breakpoint.
And I check the state of the breakpoint on the command above:
```
self.assertFalse(location.GetInjectCondition(), VALID_BREAKPOINT_LOCATION)
```
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:47
+ loc_sp->GetConditionText());
+ return false;
+ }
----------------
aprantl wrote:
> Should this return an error instead of logging and returning false?
Error is handled on the upper function, at the process level.
================
Comment at: lldb/source/Breakpoint/BreakpointInjectedSite.cpp:443
+ int64_t operand = op.getRawOperand(0);
+ expr += " src_addr = " + std::to_string(operand) +
+ ";\n"
----------------
aprantl wrote:
> This seems unnecessarily slow. Perhaps use an llvm::Twine?
As discussed, using `llvm::Twine` doesn't fit the implementation. Since this
part of the code will probably change in the future, to support more DWARF
Expression, I'll leave it as is for now, and try to come up with a more
efficient approach in the coming patches.
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
