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

[stdlib] Replace #elseif with #else for 64-bit platforms #14409

Closed
wants to merge 1 commit into from
Closed

[stdlib] Replace #elseif with #else for 64-bit platforms #14409

wants to merge 1 commit into from

Conversation

xwu
Copy link
Collaborator

@xwu xwu commented Feb 5, 2018

As discussed in #14386:

It seems unnecessarily fragile/verbose to list all supported 64-bit architectures[...]. We did go with #else in the new String implementation[...]

This PR removes the list of supported 64-bit architectures from non-test code in the standard library. (It'll be fine to continue testing only specifically enumerated architectures.)

@xwu
Copy link
Collaborator Author

xwu commented Feb 5, 2018

@swift-ci Please smoke test

@xwu xwu requested a review from lorentey February 5, 2018 15:33
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.

I think this is a clear readability win with no drawbacks. The tests remain easily greppable in case we add new archs later.

It may be a good idea to annotate all these #ifs with a standardized comment to indicate that the intention was to test for the width of a machine word.

@jrose-apple
Copy link
Contributor

jrose-apple commented Feb 5, 2018

:-( I'd push back against this. For low-level code, it's usually good to make explicit decisions about all new architectures.

EDIT: until we have a dedicated check for whatever the real query is here. (Do ILP64 arches count? LLP64?)

@gparker42
Copy link
Contributor

I agree with Jordan. Porting is hard.

The String code in #14386 was able to use #else because that clause generates a build-time error if done incorrectly. That is not the case for at least some of the changes involved here.

@lorentey
Copy link
Member

lorentey commented Feb 5, 2018

Fair enough! I agree a dedicated conditional would be much preferable.

I think Swift's Int type is defined to match the size of a pointer, so we don't need to distinguish between LP64/ILP64/LLP64 in any of these cases.

@jrose-apple
Copy link
Contributor

Yes, Int matches the size of a pointer. I just meant in general people aren't always clear about what they're actually testing for. (We had a bunch of __LP64__ in the C++ bits of the runtime that @compnerd had to fix to __POINTER_WIDTH__ == 64 to get Windows to work.)

@lorentey
Copy link
Member

lorentey commented Feb 5, 2018

What if we introduced a _pointerwidth(n) conditional, then? It'd make the intention explicit:

#if _pointerwidth(32)
  return Int(Int32(bitPattern: rand32()))
#elseif _pointerwidth(64)
  return Int(Int64(bitPattern: rand64()))		
#else
  fatalError("unimplemented")
#endif

(We currently call this word_bits in gyb.)

@jrose-apple
Copy link
Contributor

That's @xwu's #14413. As I said there, I'm reluctant to do something like that without going through Evolution. But I could be overruled.

@xwu
Copy link
Collaborator Author

xwu commented Feb 6, 2018

I'll close this PR in favor of continuing discussion on #14413.

@xwu xwu closed this Feb 6, 2018
@xwu xwu deleted the else-64-bit branch February 6, 2018 01:24
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