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

wallace added a comment. Thanks for the notice. I apologize for not doing everything correctly. I'm still trying to understand how all the different piece of the system works and I end up learning from the things that fail... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

MaskRay added a comment. commit fd868f517d2c5ca8c0f160dbec0857b14ecf74c1 should be associated with `Differential Revision: https://reviews.llvm.org/D76111`. The revert of fd868f517d2c5ca8c0f160dbec0857b14ecf74c1

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

aprantl added a comment. It looks like this test doesn't work on the bots: http://green.lab.llvm.org/green//view/LLDB/job/lldb-cmake/lastCompletedBuild//testReport/junit/lldb-api/python_api_sbenvironment/TestSBEnvironment_py/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

aprantl added a comment. Since it's late on Friday afternoon I'm taking the liberty to revert the patch to get the bots running again. Feel free to re-land with a fix or ask for another round of review if you have more questions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG2dec82652e4b: Create basic SBEnvironment class (authored by Walter Erquinigo walterme...@fb.com). Repository: rG LLVM

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

wallace updated this revision to Diff 251740. wallace added a comment. apply last suggestions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111 Files: lldb/bindings/headers.swig

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

wallace added a comment. Thanks a lot, guys. I learned a lot from you doing this patch :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111 ___ lldb-commits

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

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-commits] [PATCH] D76111: Create basic SBEnvironment class

wallace updated this revision to Diff 251472. wallace added a comment. fix grammar Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111 Files: lldb/bindings/headers.swig

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

wallace updated this revision to Diff 251470. wallace added a comment. - Added both kinds of APIs we were discussing. It will come handy for all different kind of usages - Added an SBEnvironment API for SBLaunchInfo, which will be used in https://reviews.llvm.org/D74636 - Address all sorts of

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

wallace marked 2 inline comments as done. wallace added inline comments. 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

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

clayborg added a comment. In D76111#1930783 , @labath wrote: > The new interface is ok for me, but there are two things I want to mention: > > - the iteration via `Get{Name,Value}AtIndex` is going to be slow because the > underlying map (like most maps)

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

wallace added a comment. Well, I think I'll implement both kind of accessors in the API to account for all possible cases Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111

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

labath added a comment. The new interface is ok for me, but there are two things I want to mention: - the iteration via `Get{Name,Value}AtIndex` is going to be slow because the underlying map (like most maps) does not have random access iterators. This is the problem I was trying to solve with

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

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Just some header doc cleanup before we submit. Thanks for sticking with these changes! Comment at: lldb/include/lldb/API/SBEnvironment.h:24 + + const

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

wallace updated this revision to Diff 251201. wallace added a comment. nit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/

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

wallace updated this revision to Diff 251200. wallace added a comment. Addressed Greg's comments. At the end I ended up not using APIs that use the format NAME=value, as they could be error prone. I think it's safer to use functions that work with explicit names and values. Besides, I don't

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

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. We need to determine if the objects we return are copies (from SBPlatform and SBTarget), or if they are objects that will modify the environment for the platform and target

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

wallace added a comment. Thanks for the review! 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

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

wallace updated this revision to Diff 251143. wallace added a comment. address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111 Files: lldb/bindings/headers.swig

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

[Splitting this off to not derail the review] On 14/03/2020 00:45, Greg Clayton via Phabricator wrote: > labath wrote: >> This will return a dangling pointer as Envp is a temporary object. It's >> intended to be used to pass the environment object to execve-like function. >> The current "state

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

labath added a comment. This looks pretty good overall. I have a bunch of comments, but nothing major. Could you also please rebase the other patch (D74636 ) on top of this. I think that a very common use case for this will be taking the platform environment,

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

wallace updated this revision to Diff 250649. wallace added a comment. address comments≈ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111 Files: lldb/bindings/headers.swig

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

wallace updated this revision to Diff 250651. wallace added a comment. format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111 Files: lldb/bindings/headers.swig lldb/bindings/interface/SBEnvironment.i

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

wallace updated this revision to Diff 250650. wallace added a comment. . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111 Files: lldb/bindings/headers.swig lldb/bindings/interface/SBEnvironment.i

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

clayborg added inline comments. Comment at: lldb/bindings/interface/SBPlatform.i:197-198 +lldb::SBEnvironment +GetEnvironment(); + What does it mean to get the environment from a platform? Fetching it from the remote platform as what ever binary was

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

wallace added a comment. Thanks a lot for the feedback! I was thinking about adding features to this class as needed, but definitely I should make this API be more like a map Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/

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

jingham added a comment. Yes, a key/value approach would be a better API. Looks like the Environment class would make that pretty easy to wrap up, as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111

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

labath added a comment. I'm not sure that this is the right API to represent an environment. The environment is more like a dictionary/map than an array. (The internal Environment object *is* a map, though this does not immediately mean the SB one should be too). Even for your most immediate

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

jingham requested changes to this revision. jingham added a comment. This revision now requires changes to proceed. Thanks for doing this, it will be useful! I'll let Jonas comment on whether the Reproducer bindings are right, though it looks fine to me. One of the very common uses of an

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

wallace updated this revision to Diff 250108. wallace added a comment. format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76111/new/ https://reviews.llvm.org/D76111 Files: lldb/bindings/headers.swig lldb/bindings/interface/SBEnvironment.i

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

wallace created this revision. wallace added reviewers: labath, clayborg. Herald added subscribers: lldb-commits, mgorny. Herald added a project: LLDB. Inspired by https://reviews.llvm.org/D74636, I'm introducing a basic version of Environment in the API. More functionalities can be added as