Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(46)

Issue 13240043: Benign edits to PEP 446 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 3 months ago by GvR
Modified:
11 years, 3 months ago
Reviewers:
Visibility:
Public.

Description

Benign edits to PEP 446

Patch Set 1 #

Patch Set 2 : Slight tweaks after reviewing all changes #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+73 lines, -73 lines) Patch
M pep-0446.txt View 1 14 chunks +73 lines, -73 lines 7 comments Download

Messages

Total messages: 1
GvR
11 years, 3 months ago (2013-08-26 21:29:25 UTC) #1
https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt
File pep-0446.txt (right):

https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode25
pep-0446.txt:25: descriptors.
I'd add at this point:

We are aware of the code breakage this is likely to cause, and doing it anyway
for the good of mankind. (Details in the section "Backward Compatibility"
below.)

https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode80
pep-0446.txt:80: inheritable handles are inherited by the child process.
Maybe mention here that this also affects the subprocess module?  (You mention
it later, but it's important to realize at this point.)

https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode437
pep-0446.txt:437: condition.
As C-F Natali pointed out, this is not actually a problem, because after fork()
only the main thread survives.  Maybe just delete this paragraph?

https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode450
pep-0446.txt:450: parameter is a non-empty list of file descriptors.
Well, it could pass closefrom() the max of the given list and manually close the
rest. This would be useful if the system max is large but none of the FDs given
in the list is. (This would be more complex code but it would address the issue
for most programs.)

https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode486
pep-0446.txt:486: * ``socket.socketpair()``
I would call out that dup2() is intentionally not in this list, and add a
rationale for that omission below.

https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode528
pep-0446.txt:528: by default, but non-inheritable if *inheritable* is ``False``.
This might be a good place to explain the rationale for this exception.

https://codereview.appspot.com/13240043/diff/3001/pep-0446.txt#newcode538
pep-0446.txt:538: descriptors).
I would say it should not be changed because the default is still better. :-)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b