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.
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
gh-114099: Additions to standard library to support iOS #117052
gh-114099: Additions to standard library to support iOS #117052
Changes from 14 commits
760751b
b41f9a4
0f3e9bc
1a9d965
c7fe185
732c4da
97a081d
95c367d
3d6d875
d12cfa0
95d11fb
48f4c1a
f584d29
9515f75
cf0b5ff
96aa042
24b3662
a4e09c9
41a3c1a
84ba760
4194849
9a91933
e6550b7
8654376
7a4dcaf
4451326
4386a7a
c1a1f0b
61559ac
abc2034
096078a
44bbf79
7419002
c121d5f
1ac4b26
857a0c3
aa65dc2
61e51ff
2ee4aba
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.
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.
As above: iOS always has
_multiarch
, andget_platform_ios()
is only invoked byplatform.ios_ver()
, after being gated to be iOS-specific.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.
snake_case
for this and the other variables here, or is it more important to mirror theobjc
names?https://peps.python.org/pep-0008/#function-and-variable-names
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 intention was to mirror the objc naming, so that it's a little easier to see that the name comes from an "alien space", and might have some additional considerations - a sort of very weak Hungarian notation.
In terms of pure functionality, it doesn't have any impact on the operation of the code. Any variable name will do; this naming convention is entirely as an affordance to future readers. For my money, the "hey, that's weird - they're not camel case" reaction is mildly desirable in this case.
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.
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.
Because of the way the build works for iOS, I think we can assume that _multiarch is always present.
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.
Confirming: iOS builds will always have
_multiarch
set (because of this section of configure.ac); and this block of logic will only be invoked on iOS as it's part of aget_ios()
method.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.
That is true for CPython, but it may change for different implementations that keep our stdlib. It's not a big deal, but generally I avoid adding a hard dependency on implementation details. This is certainly not a blocker, just a possible improvement.
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.
Avoiding hard dependencies on implementation details definitely makes sense; However, I'm not sure if using a fallback is entirely safe. Without any additional changes, using a fallback value for
_multiarch
will result in:ios-12.0-
, regardless of architecture or simulator statusThe second point would at least complicate, and possibly prevent any binary module from loading; the first would lead to misleading or incorrect behavior.
On that basis, I'd argue it would be safer to fail early, providing a clear indicator that something isn't configured correctly (or, at least, not configured the way that the CPython implementation expects). However, it's also a relatively straightforward change, so I'm happy to be overruled on this.
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.
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.
Again; this is iOS specific code.