labath accepted this revision.
labath added a comment.
This looks good to me. I still have some doubts about the ConstStringification
of the returned values (I am not a fan of constifying everything), but I don't
want to block this review for that.
================
Comment at: lldb/source/API/SBEnvironment.cpp:62-73
+ ConstString(std::next(m_opaque_up->begin(), index)->first())
+ .AsCString(""));
+}
+
+const char *SBEnvironment::GetValueAtIndex(size_t index) {
+ LLDB_RECORD_METHOD(const char *, SBEnvironment, GetValueAtIndex, (size_t),
+ index);
----------------
wallace wrote:
> clayborg wrote:
> > labath wrote:
> > > I don't think these need to const-stringified now, given that they are
> > > backed by the underlying map. We already have functions which return
> > > pointers to internal objects (e.g. SBStream::GetData).
> > >
> > > @clayborg?
> > That's a tough one. I would like to think that any "const char *" that
> > comes back from an API that returns strings could just be pointer compared
> > if needed. So I like the idea that for strings that comes out of the API
> > that they are all const-ed and could easily be compared. I am fine with
> > SBStream::GetData() returning its own string because _it_ owns the data. So
> > I would vote to ConstString() any returned results
> I'm adding a Clear method. With that, the backing char* might be wiped out
> and the python reference to this string might be invalid unless we use
> ConstString
> I am fine with SBStream::GetData() returning its own string because _it_ owns
> the data.
I am afraid I don't see the distinction here. SBStream owns a Stream (via a
unique_ptr) and the stream data is inside that. An SBEnvironment has a
unique_ptr<Environment>, and the strings are inside of that.
> I'm adding a Clear method. With that, the backing char* might be wiped out
> and the python reference to this string might be invalid unless we use
> ConstString
Python will be fine if as it will copy the string as soon as it gets its hand
on it. C++ users could be affected by that, but, this is not something that
should come across as surprising to them (and indeed, the same thing will
happen with SBStream::GetData).
================
Comment at: lldb/source/API/SBEnvironment.cpp:26
+
+ m_opaque_up = clone(rhs.m_opaque_up);
+}
----------------
move this into the initializer list?
================
Comment at: lldb/source/API/SBEnvironment.cpp:29
+
+SBEnvironment::SBEnvironment(const Environment &rhs)
+ : m_opaque_up(new Environment(rhs)) {}
----------------
super-nit: if you make this constructor accept a `Environment rhs` (without the
`const &` part) and then do the initialization as `new
Environment(std::move(rhs))`, you will avoid copying the environment in the
expressions like `SBEnvironment(Environment(whatever))`. Currently that will
create one `Environment` object to initialize the `rhs` argument, and then a
second one when copying things to `m_opaque_up`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76111/new/
https://reviews.llvm.org/D76111
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
