Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
gh-127604: Add C stack dumps to
faulthandler
#128159New 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
gh-127604: Add C stack dumps to
faulthandler
#128159Changes from all commits
0ebdc69
0ccf9fb
52945e5
9bdd802
66f2641
0591013
64707a2
8a5b784
4f09358
e1aa619
6b515d3
c69ee9d
f08b1dd
dbb6d25
faf1a3e
ec832aa
524f167
dd08bcb
bda3dcd
a3564a5
f3fcea1
db97dd2
c17457f
d5f7d4b
e79e661
0c84f8a
8198997
0f670f0
079f186
892a085
896abd1
cab079e
1784071
3304e2a
aa97f24
50b4964
533b1db
58b3580
f62dac8
4feaf09
c95369f
95833e7
3e2701d
56127fa
b70bd43
7a070ab
e9c3d7c
195a539
9dd6c3b
ce9c39f
c344ad7
e899792
b136f71
2c381b9
52c0748
bd47026
07a20d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is totally unrelated, but many times have I been using a construction of the form:
I'm wondering if it makes sense to have a helper that would be equivalent to the above:
I'm pretty sure we can use this kind of pattern in the test directory quite a lot, and it can also be useful for other libs, but how about we add this kind of helper to
test.support
at least? (maybe, let's make it first a mixin class before implementing it onunittest.TestCase
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that could be interesting. What happens when
x1s
andxns
aren't the same length?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, it doesn't matter? we're computing a product not a zip().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm misunderstanding it. Is:
supposed to be equivalent to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The rationale behind my suggestion is as follows:
We're already having 3 levels of indentation. Counting the class indentation and method indentation we're just having 20 whitespaces before wer start writing real code...:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, if we're pretty short on the keywords passed to subTest() if we want to keep everything under 80 chars (so it's a product() + subTest() combo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I get it--the
a
doesn't change with each iteration ofcases
. I think that could be an interesting addition, but we might also run into PEP-20:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One obvious way, probably but this obvious way makes the code really harder to read IMO. Anyway, this is a bit off-topic and I don't know how much we can use it (but I've personally needed more than once to be able to use products for testing cases).
Note that importlib's tests already has some "parametrize" decorator as in Pytest, so we could also move it up to test.support instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using two blank lines to separate classes in this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like there are any other classes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, most of the file is using 1-blank line. Up to you then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is better to use atomics here? AFAIU volatile have different behavior for msvc for different platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be, but regardless, that should be done in a different PR.