Skip to content

Make Errno enum values a set union of all possible LibC Errno values#9573

Draft
oprypin wants to merge 1 commit into
crystal-lang:masterfrom
oprypin:errall
Draft

Make Errno enum values a set union of all possible LibC Errno values#9573
oprypin wants to merge 1 commit into
crystal-lang:masterfrom
oprypin:errall

Conversation

@oprypin
Copy link
Copy Markdown
Member

@oprypin oprypin commented Jul 4, 2020

So, if an error name is available on at least one platform, it is possible to refer to it on all platforms, even if its value is INVALID = -1.
That is in accordance with Crystal's planned API guarantee: if it compiles on one platform, it should compile on any platform.
This is not problematic because all usages should be of the form if err == Errno::ENOATTR, and if such a value is not possible to encounter on a given platform, it's expected that the equality will never be true. Additionally, if the platform in the future gains more possible error codes that we're not aware of yet, the equality with them will also be false (they are not clumped into INVALID but rather have their normal integer value, just that it's not known to the enum -- to_s and such).

Also update and sort the list of all errors accordingly.


Prior discussion: #8885 (comment)
This is an alternative to #9523 (comment)

So, if an error name is available on at least one platform, it is possible to refer to it on all platforms, even if its value is `INVALID = -1`.
That is in accordance with Crystal's planned API guarantee: if it compiles on one platform, it should compile on any platform.
This is not problematic because all usages should be of the form `if err == Errno::ENOATTR`, and if such a value is not possible to encounter on a given platform, it's expected that the equality will never be true. Additionally, if the platform in the future gains more possible error codes that we're not aware of yet, the equality with them will also be false (they are not clumped into `INVALID` but rather have their normal integer value, just that it's not known to the `enum` -- `to_s` and such).

Also update and sort the list of all errors accordingly.
@aravindavk
Copy link
Copy Markdown

Looks good. Thanks for all the clarification.

This is an alternative to #9523 (comment)

Not an alternative. Because that PR is supposed to add the missing error numbers. I will rebase my PR to add new error codes to respective platform-specific files and to src/errno.cr file.

@oprypin
Copy link
Copy Markdown
Member Author

oprypin commented Jul 4, 2020

This is an alternative to #9523 (comment)

Not an alternative

Alternative to the linked comment, not the PR entirely.


I will rebase my PR

You have a shortcut towards that, as I already cut out the relevant part of it here #9523 (comment)

  • git fetch origin 467061e && git reset --hard FETCH_HEAD .

-- you could also try the same with the merged state (c5fc39d) but GitHub has no clean way to represent dependent PRs so that'd just be a mess.

But probably best to wait until discussion on this one settles down, as my opinion has no guarantee of being selected as correct.

@aravindavk
Copy link
Copy Markdown

You have a shortcut towards that, as I already cut out the relevant part of it here #9523 (comment)
-- you could also try the same with the merged state (c5fc39d) but GitHub has no clean way to represent dependent PRs so that'd just be a mess.

No issues, I can handle the rebase. Thanks

Comment thread src/errno.cr
# The integer values of these errors are platform-specific and should not be directly relied on.
# If an error code is not applicable on the current platform, its value will be `INVALID`.
#
# Intended usages are of the form `if exc.os_error == Errno::ENOATTR`: with this we are checking
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we mention the alternative predicate method: exc.os_error.enoattr?

@asterite asterite added the DON'T MERGE Don't merge yet! This needs further discussion. label Jul 9, 2020
@asterite
Copy link
Copy Markdown
Member

asterite commented Jul 9, 2020

Please don't merge this yet. We'll discuss it in the next core team meeting. And we'll probably go with the non-portable way.

@oprypin
Copy link
Copy Markdown
Member Author

oprypin commented Dec 25, 2020

How did the meeting go? 😅

In any case, seems like the label "pr:on-hold-do-not-merge" is more appropriate than this unique one

@oprypin oprypin marked this pull request as draft June 9, 2021 21:55
@oprypin oprypin added pr:on-hold-do-not-merge This PR is on hold. Do not merge. and removed DON'T MERGE Don't merge yet! This needs further discussion. labels Jun 9, 2021
@HertzDevil
Copy link
Copy Markdown
Contributor

^ What happened to this? I was thinking if POSIX signals should do this same thing on Windows MSVC

@straight-shoota
Copy link
Copy Markdown
Member

I don't recall whether this was ever discussed in a Core Team meeting, or whatever the outcome was 🙈

@HertzDevil
Copy link
Copy Markdown
Contributor

HertzDevil commented Mar 8, 2023

Signal currently lists all the possible flags in the documentation instead. The docs generator runs on a Linux host (evidenced by the documented value of Crystal::DESCRIPTION), so the API docs list all Signal members that are defined on Linux, and Signal::BREAK is missing.

I don't know if the same approach is viable for Errno because this enum is much larger than Signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants