Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-30695: Add set_nomemory(start, stop) to _testcapi #2406

Merged
merged 3 commits into from Jul 1, 2017

Conversation

@xdegaye
Copy link
Contributor

xdegaye commented Jun 26, 2017

No description provided.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Jun 26, 2017

@xdegaye, thanks for your PR! By analyzing the history of the files in this pull request, we identified @larryhastings, @pitrou and @serhiy-storchaka to be potential reviewers.

typedef struct fm_filter_t {
void *data;
int (*has_memory) (struct fm_filter_t *);
} fm_filter_t;

This comment has been minimized.

Copy link
@vstinner

vstinner Jun 26, 2017

Member

This structure seems very generic, whereas the implementation is very specific.

Do we really need a configurable callback? Why not always call the same filter function and just store somewhere counters?

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 26, 2017

Author Contributor

I must have misunderstood our discussion on issue 30695. I thought inclusion of pyfailmalloc in _testcapi (with its random start counter) was considered so that the test suite could be run forever until a problem with memory was detected. In that case a generic filter structure is useful. Otherwise I agree that the PR should be changed toward a simpler implementation.

typedef struct {
int count;
int start;
int stop;

This comment has been minimized.

Copy link
@vstinner

vstinner Jun 26, 2017

Member

I suggest to use Py_ssize_t here, to prevent integer overflows.

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 26, 2017

Author Contributor

Will change the type of 'count' to Py_ssize_t.

Copy link
Member

serhiy-storchaka left a comment

Concur with Victor. This code looks overcomplicated.

fm_remove_filter(void)
{
if (FmFilter != NULL) {
if (FmFilter->data != NULL) {

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 26, 2017

Member

You can call PyMem_RawFree() with NULL.

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 26, 2017

Author Contributor

Thanks. I will change that.

* to 0 (default) in which case allocation failures never stop. */
data->count = 0;
data->stop = 0;
if (! PyArg_ParseTuple(args, "i|i", &data->start, &data->stop)) {

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 26, 2017

Member

Why use a space after !?

This comment has been minimized.

Copy link
@xdegaye

xdegaye Jun 26, 2017

Author Contributor

For readability, a personal preference. But I see that the usage in Python code is overwhelmingly in favor of no space (5382 against 61 in the .c files), so I will change that.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jun 26, 2017

This PR basically implements the same feature than pyfailmalloc. I don't see the need of including this PR and pyfailmalloc. So I suggest to focus on your PR!

@xdegaye

This comment has been minimized.

Copy link
Contributor Author

xdegaye commented Jun 29, 2017

Not sure if this PR is approved by the reviewers after the last changes.

@xdegaye xdegaye merged commit 85f6430 into python:master Jul 1, 2017
2 checks passed
2 checks passed
bedevere/issue-number Issue number 30695 found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xdegaye xdegaye deleted the xdegaye:bpo-30695 branch Jul 1, 2017
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Oct 23, 2017

Thanks @xdegaye for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒🤖

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Oct 23, 2017

GH-4083 is a backport of this pull request to the 3.6 branch.

miss-islington added a commit to miss-islington/cpython that referenced this pull request Oct 23, 2017
xdegaye added a commit that referenced this pull request Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.