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

[WIP] [Parse] Add _pointer_bit_width platform condition #14413

Closed
wants to merge 3 commits into from
Closed

[WIP] [Parse] Add _pointer_bit_width platform condition #14413

wants to merge 3 commits into from

Conversation

xwu
Copy link
Collaborator

@xwu xwu commented Feb 5, 2018

Instead of explicitly listing each platform when testing for the native word size (which, per documentation, is the bit width of Int), add an explicit (underscored) platform conditional.

This supersedes the solution discussed in #14386 and #14409.

@xwu
Copy link
Collaborator Author

xwu commented Feb 5, 2018

@swift-ci Please smoke test Linux platform

@jrose-apple
Copy link
Contributor

jrose-apple commented Feb 5, 2018

At the same time as I objected to the other PR, I'm leery of adding this even as an underscored condition because people will start using it. Seems like someone should just take this through Evolution.

@xwu xwu changed the title [WIP] [Parse] Add _native_word_size platform condition [WIP] [Parse] Add _pointer_bit_width platform condition Feb 6, 2018
@xwu
Copy link
Collaborator Author

xwu commented Feb 6, 2018

@swift-ci Please clean smoke test Linux platform

@xwu
Copy link
Collaborator Author

xwu commented Feb 6, 2018

@jrose-apple I see your point. I think if there's broad consensus that this is necessary for the stdlib, however, then an underscored condition wouldn't be terrible as long as we intend to make it public eventually anyway.

At the moment I'm rather flummoxed by the technical issue that this seemingly straightforward addition doesn't seem to work. I wonder if it's the integer values that aren't being parsed correctly.

@jrose-apple
Copy link
Contributor

At the moment I'm rather flummoxed by the technical issue that this seemingly straightforward addition doesn't seem to work. I wonder if it's the integer values that aren't being parsed correctly.

Yeah, the current grammar for platform conditions always expects a single identifier as the argument. You could use _32 and _64 for now, which not only sidesteps the implementation issue but matches _runtime(_ObjC).

@xwu
Copy link
Collaborator Author

xwu commented Feb 6, 2018

@swift-ci Please smoke test Linux platform

Endianness,
/// The active arch target pointer bit width (_32 or _64)
PointerBitWidth,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cf. __POINTER_WIDTH__ in Clang, getArchPointerBitWidth in LLVM, target_pointer_width in Rust.

@xwu
Copy link
Collaborator Author

xwu commented Feb 6, 2018

@swift-ci Please smoke test OS X platform

@CodaFi
Copy link
Contributor

CodaFi commented Dec 6, 2019

@xwu If you still want to pursue this, could you rebase it and take it through evolution?

Also, if you wouldn't mind adding a Parse test for a bogus condition value.

@CodaFi
Copy link
Contributor

CodaFi commented Apr 10, 2020

Alright, it's been 4 months without a reply. I'm closing this due to age and inactivity.

@CodaFi CodaFi closed this Apr 10, 2020
@xwu
Copy link
Collaborator Author

xwu commented Apr 11, 2020

It's entirely slipped my mind. Will revisit this topic later in the year, time permitting. Thanks @CodaFi!

@AnthonyLatsis AnthonyLatsis removed the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants