[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Pavel Labath via Phabricator via lldb-commits]

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