fjricci abandoned this revision.
fjricci added a comment.
Committed.
http://reviews.llvm.org/D18459
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
sas added a comment.
Committed as r264476.
http://reviews.llvm.org/D18459
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
clayborg accepted this revision.
clayborg added a comment.
Looks good. Thanks for taking the time to get this working right.
http://reviews.llvm.org/D18459
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailm
fjricci updated this revision to Diff 51699.
fjricci added a comment.
Fix whitespace
http://reviews.llvm.org/D18459
Files:
include/lldb/API/SBCommandReturnObject.h
scripts/interface/SBCommandReturnObject.i
source/API/SBCommandReturnObject.cpp
Index: source/API/SBCommandReturnObject.cpp
=
fjricci updated this revision to Diff 51698.
fjricci added a comment.
Use function overloading
http://reviews.llvm.org/D18459
Files:
include/lldb/API/SBCommandReturnObject.h
scripts/interface/SBCommandReturnObject.i
source/API/SBCommandReturnObject.cpp
Index: source/API/SBCommandReturnOb
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
I would prefer to do this with overloading if possible. See inlined comments.
Comment at: include/lldb/API/SBCommandReturnObject.h:87-91
@@ -86,7 +86,7 @@
fjricci updated this revision to Diff 51680.
fjricci added a comment.
Correct whitespace change
http://reviews.llvm.org/D18459
Files:
include/lldb/API/SBCommandReturnObject.h
scripts/interface/SBCommandReturnObject.i
source/API/SBCommandReturnObject.cpp
Index: source/API/SBCommandReturnO
fjricci updated this revision to Diff 51678.
fjricci added a comment.
Extend SBCommandReturnObject API to allow for taking file ownership
http://reviews.llvm.org/D18459
Files:
include/lldb/API/SBCommandReturnObject.h
scripts/interface/SBCommandReturnObject.i
source/API/SBCommandReturnObje
clayborg added a comment.
We don't necessarily need to expose this new API to swig. We get to pick and
choose what we expose to Python in the SBCommandReturnObject.i file. So leave
the .i file as is and use %rename to specify "transfer_ownership = true". Will
that work?
http://reviews.llvm.or
fjricci added a comment.
Actually, I think this may open a new can of worms. Adding that flag to the API
doesn't necessarily achieve what we want. The current bug is that we take a
Python file (owned by the Python caller), and we know that the Python caller
does not want to transfer ownership o
fjricci added a comment.
I think that would be sufficient to solve the problem.
I could use a `%rename` to rename the `SetImmediateOutputFile (FILE *fh)` call
to a private function in `%extend`, which will then call
`SetImmediateOutputFile (FILE *fh, transfer_ownership=true)` instead.
Does tha
clayborg requested changes to this revision.
clayborg added a comment.
Seems like we need to just add the following to the API:
void
SetImmediateOutputFile (FILE *fh, bool transfer_ownership);
void
SetImmediateErrorFile (FILE *fh, bool transfer_ownership);
This should solve our proble
fjricci added a comment.
It appears that `%extend` directives only have access to public class members.
So my idea won't work. I'll try to overload with a fd call, presumably that
call can be exposed in the API? Or is it better if it isn't?
The difficulty with overloading is that, for overloade
As long as it only changes the private API and not the public it's probably
fine, if you think that will work feel free to give it a try and upload a
new patch
On Fri, Mar 25, 2016 at 9:12 AM Francis Ricci wrote:
> fjricci added a comment.
>
> Would we want this overloaded method taking an int (f
fjricci added a comment.
Would we want this overloaded method taking an int (fd) instead of a FILE* to
be present in the API? Or hidden somehow? I'm new to swig, but it looks like
python-extensions.swig includes some private extensions to the
CommandReturnObject class already.
Overloading with
Could there be an overload of SBCommandReturnObject constructor that takes
an fd instead of a FILE*, and pass through to the private method which does
its own duping? Then this typemap could be changed to convert to int
instead of to FILE*
Also I'm OOO until tomorrow now, so I can't look at the co
fjricci added inline comments.
Comment at: scripts/Python/python-typemaps.swig:532
@@ -531,3 +524,1 @@
- file.Clear();
-}
}
zturner wrote:
> zturner wrote:
> > fjricci wrote:
> > > The problem is that here, we save the `FILE*` (`$1`) and let the `File
fjricci added inline comments.
Comment at: scripts/Python/python-typemaps.swig:532
@@ -531,3 +524,1 @@
- file.Clear();
-}
}
The problem is that here, we save the `FILE*` (`$1`) and let the `File`
(`file`) go out of scope. So the `File` gets destructe
zturner added inline comments.
Comment at: scripts/Python/python-typemaps.swig:532
@@ -531,3 +524,1 @@
- file.Clear();
-}
}
zturner wrote:
> fjricci wrote:
> > The problem is that here, we save the `FILE*` (`$1`) and let the `File`
> > (`file`) go ou
zturner added inline comments.
Comment at: scripts/Python/python-typemaps.swig:532
@@ -531,3 +524,1 @@
- file.Clear();
-}
}
fjricci wrote:
> The problem is that here, we save the `FILE*` (`$1`) and let the `File`
> (`file`) go out of scope. So the `F
fjricci added a comment.
When we pass the `FILE*` to `SetImmediateOutputFile`, it then generates a new
`File` from this `FILE*`, but doesn't take ownership, because the
transfer_ownership flag is always false. And the `File` destructor only closes
files if it has ownership of the `FILE*`.
htt
zturner added a comment.
if (!StreamIsValid())
{
if (DescriptorIsValid())
{
const char *mode = GetStreamOpenModeFromOptions (m_options);
if (mode)
{
if (!m_should_close_fd)
{
// We must duplicate the file d
fjricci added a comment.
Because python 3 doesn't use `FILE*` as the underlying implementation anymore,
I think the only way to make this work is to continue to use `dup()` to make
the `FILE*`, but then to actually keep track of the ownership of the `FILE*`.
This can't be done inside `PythonFil
zturner requested changes to this revision.
zturner added a comment.
This revision now requires changes to proceed.
This patch won't work. `PyFile_AsFile` doesn't exist on Python 3. Anything
that you need done to make this work has to be wrapped up in `PythonFile` class
as it was before, becau
fjricci created this revision.
fjricci added reviewers: clayborg, granata.enrico, zturner.
fjricci added subscribers: sas, lldb-commits.
This reverts some of the changes made by:
r257644
r250530
r250525
The changes made in these patches result in leaking the FILE* passed
to SetImmediateOutputFile
25 matches
Mail list logo
