Skip to content

Fix for memory leak and doctest#96

Merged
lelit merged 9 commits into
python-rapidjson:masterfrom
enquos:master
Mar 31, 2018
Merged

Fix for memory leak and doctest#96
lelit merged 9 commits into
python-rapidjson:masterfrom
enquos:master

Conversation

@silviot

@silviot silviot commented Mar 26, 2018

Copy link
Copy Markdown
Contributor

The __init__ constructor of the RawJSON class can be invoked multiple times (silly, but possible).

So the code that takes care of the reference count of the previous value probably needs to stay.

I changed the way doctests are run in travis, so that the make the build fail. This uncovered a problem in a doctest: the timezone of the test environment was relevant, so tests were passing on my machine but failing on Travis. I added the DM_NAIVE_IS_UTC flag to fix them.

I added tests with the only python-debug version I could find on travis-ci: 3.4.

I'm not sure how these changes interact with the distribution build though.

@lelit

lelit commented Mar 26, 2018

Copy link
Copy Markdown
Contributor

Uhm, I'm quite surprised of that usa^H^H^Habuse of the RawJSON constructor: what's the use case?

@lelit

lelit commented Mar 26, 2018

Copy link
Copy Markdown
Contributor

Unless I'm missing the obvious, I think a better way would be using tp_new, as done for the other types, so that it's clear that that strangeness is not supported.

@silviot

silviot commented Mar 27, 2018

Copy link
Copy Markdown
Contributor Author

There's no use case for that kind of abuse.

This was my first attempt at Python in C-land (c++ actually) so I'm still learning. Reading the docs I found that was recommended, as you never can tell how users will use your code, and you should strive to not break in all cases.

I agree initialization of the RawJSON class is better done in the tp_new function.

I updated the PR with the switch tp_init --> tp_new and a fix for cibuildwheel step.

Comment thread rapidjson.cpp Outdated
static int
RawJSON_init(RawJSON* self, PyObject* args, PyObject* kwds)
static PyObject *
RawJSON_new(PyTypeObject *type, PyObject *args, PyObject *kwds)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow overall stylistic convention, using PyObject* foo instead of PyObject *foo: although it's not my preferred style, but I tried to adhere to the one originally used by Ken

Comment thread rapidjson.cpp Outdated
self->value = value;
Py_XDECREF(tmp);
}
((RawJSON*)self)->value = value;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

((RawJSON*) self)->value

@lelit

lelit commented Mar 27, 2018

Copy link
Copy Markdown
Contributor

Thanks, sorry for being so picky ;-)

@silviot

silviot commented Mar 27, 2018

Copy link
Copy Markdown
Contributor Author

@lelit np! I understand :)

Thanks for maintaining the project!

@lelit lelit merged commit 5cc1441 into python-rapidjson:master Mar 31, 2018
@lelit

lelit commented Mar 31, 2018

Copy link
Copy Markdown
Contributor

Thanks Silvio, I merged this and released v0.5.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants