bpo-30695: Add set_nomemory(start, stop) to _testcapi#2406
bpo-30695: Add set_nomemory(start, stop) to _testcapi#2406xdegaye merged 3 commits intopython:masterfrom xdegaye:bpo-30695
Conversation
|
@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. |
Modules/_testcapimodule.c
Outdated
| typedef struct fm_filter_t { | ||
| void *data; | ||
| int (*has_memory) (struct fm_filter_t *); | ||
| } fm_filter_t; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Modules/_testcapimodule.c
Outdated
| typedef struct { | ||
| int count; | ||
| int start; | ||
| int stop; |
There was a problem hiding this comment.
I suggest to use Py_ssize_t here, to prevent integer overflows.
There was a problem hiding this comment.
Will change the type of 'count' to Py_ssize_t.
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Concur with Victor. This code looks overcomplicated.
Modules/_testcapimodule.c
Outdated
| fm_remove_filter(void) | ||
| { | ||
| if (FmFilter != NULL) { | ||
| if (FmFilter->data != NULL) { |
There was a problem hiding this comment.
You can call PyMem_RawFree() with NULL.
There was a problem hiding this comment.
Thanks. I will change that.
Modules/_testcapimodule.c
Outdated
| * 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)) { |
There was a problem hiding this comment.
Why use a space after !?
There was a problem hiding this comment.
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.
|
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! |
|
Not sure if this PR is approved by the reviewers after the last changes. |
|
Thanks @xdegaye for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
GH-4083 is a backport of this pull request to the 3.6 branch. |
(cherry picked from commit 85f6430)
7 participants
No description provided.