labath added inline comments.
================
Comment at: lldb/source/Core/Address.cpp:739
- s->PutCString(", location = ");
- var_sp->DumpLocationForAddress(s, *this);
- s->PutCString(", decl = ");
----------------
zequanwu wrote:
> labath wrote:
> > This place was the only caller of Variable::DumpLocationForAddress. What if
> > we, instead of duplicating its logic here, just change the function to do
> > what we want?
> > The interface could be as simple as `Variable::DumpLocations(Stream&,
> > Address)` where, if one provides an invalid Address, then all locations get
> > dumped, and one can request a specific location by providing a concrete
> > address.
> > We might even do something similar for the DWARFExpression class, and avoid
> > the filter callbacks completely.
> I just moved the filter inside `Variable::DumpLocations`, because we still
> need to pass the filter callback to `DWARFExpression::DumpLocaitons` inside
> `Variable::DumpLocaitons` to choose dumping all ranges + locations or just
> one range location.
Looking at it now, I think it'd be better to inline the callback all the way
into DWARFExpression::DumpLocations. The callback is more flexible, but I'm not
convinced that we need that flexibility, and the interface of the callback is
pretty complex (two bool return values). I believe it would be sufficient if
the function accepted an extra address argument (as an addr_t probably), and
used an invalid address (LLDB_INVALID_ADDRESS) to mean "dump all locations".
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2684
+ }
+ bool is_first = true;
+ auto callback = [&](llvm::DWARFLocationExpression loc) -> bool {
----------------
llvm::ListSeparator separator;
================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2692-2693
+ m_data.GetAddressByteSize());
+ if (!is_first)
+ s->PutCString(", ");
+ s->PutCString("[");
----------------
s->AsRawOstream() << separator;
================
Comment at: lldb/source/Symbol/Variable.cpp:449
+bool Variable::DumpLocations(Stream *s, lldb::DescriptionLevel level,
+ lldb::addr_t func_load_addr, ABI *abi,
+ const Address &address) {
----------------
Can we keep `abi` and `func_load_addr` computations in this function? (It
probably was easier to make them arguments starting from the last version of
your patch, but putting the code here would reduce the diff from HEAD and also
make the interface simpler).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119963/new/
https://reviews.llvm.org/D119963
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
