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

Add _pointerBitWidth platform condition #41534

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

drexin
Copy link
Contributor

@drexin drexin commented Feb 23, 2022

The original PR was #14413, but was seemingly abandoned. Proposal is forthcoming.

rdar://107070345

@drexin drexin marked this pull request as draft February 23, 2022 23:06
@drexin
Copy link
Contributor Author

drexin commented Feb 23, 2022

CC: @xwu

@drexin
Copy link
Contributor Author

drexin commented Feb 23, 2022

@swift-ci test

@xwu
Copy link
Collaborator

xwu commented Feb 23, 2022

Fantastic to see this resurrected. As you deduced correctly, I never did have the time to return to this (most particularly, the proposal process), but it would be lovely to have and I'm glad to see you pick it up.

@@ -356,6 +364,25 @@ std::pair<bool, bool> LangOptions::setTarget(llvm::Triple triple) {
break;
}

// Set the "_native_word_size" platform condition.
Copy link
Contributor

@CodaFi CodaFi Feb 25, 2022

Choose a reason for hiding this comment

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

Nit:

Suggested change
// Set the "_native_word_size" platform condition.
// Set the "_pointerBitWidth" platform condition.

But also could you document where these particular target arches come from so we can keep things in sync in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure what you mean by "where these particular target arches come from". It uses the same targets as the rest of the code in this file.

@drexin drexin force-pushed the wip-word-size-condition branch from 87f9ad4 to 61f3518 Compare February 25, 2022 18:03
@drexin drexin changed the title Add _nativeWordSize platform condition Add _pointerBitWidth platform condition Mar 9, 2022
@drexin drexin force-pushed the wip-word-size-condition branch from 61f3518 to ddd5479 Compare August 2, 2022 17:53
@drexin
Copy link
Contributor Author

drexin commented Aug 2, 2022

@swift-ci smoke test

@@ -7,7 +7,7 @@
let i: Int = "Hello"
#endif

#if arch(arm) && os(Android) && _runtime(_Native) && _endian(little)
#if arch(arm) && os(Android) && _runtime(_Native) && _endian(little) && _pointerBitWidth(_32)
Copy link
Member

Choose a reason for hiding this comment

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

This seems pointless if it's not possible to use other pointer sizes on platforms like Android armv7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. In what cases would you want other pointer sizes on armv7?

Copy link
Member

Choose a reason for hiding this comment

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

My point is that isn't arch(arm) here redundant with _pointerBitWidth(_32), ie the former always implies the latter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, but the whole point of this test is to check that all of these are properly set. If any of them wasn't, the test would fail.

Copy link
Member

Choose a reason for hiding this comment

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

OK, so the goal is to just lump together all compile-time properties of this platform, even though some of them are redundant so nobody would ever use some combos of those checks together, got it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not quite. It is to ensure that if we are compiling for armv7-unknown-linux-androideabi, all of the expected conditions hold. We have these tests for all targets.

@drexin drexin force-pushed the wip-word-size-condition branch from ddd5479 to 6881b41 Compare March 21, 2023 22:07
@drexin
Copy link
Contributor Author

drexin commented Mar 21, 2023

@swift-ci test

@drexin drexin marked this pull request as ready for review March 21, 2023 22:07
Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

This has been long overdue! Thank you so much @xwu & @drexin.

@drexin drexin merged commit 294342a into swiftlang:main Mar 22, 2023
@xwu
Copy link
Collaborator

xwu commented Mar 22, 2023

Wooooo thanks @drexin for making this a reality!

etcwilde pushed a commit to etcwilde/swift that referenced this pull request Apr 19, 2023
* Add _pointerBitWidth platform condition

* Fixes after rebasing

---------

Co-authored-by: Xiaodi Wu <[email protected]>
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.

6 participants