-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Separate SystemError
from SysError
#9737
Conversation
4332d69
to
5f2fa37
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
src/libutil/error.hh
Outdated
/** | ||
* A "portable" error number. | ||
* | ||
* It has to be big enough for all platforms: | ||
* | ||
* - Unix: `int` (perhaps `int32_t`) | ||
* - Windows: `DWORD` (which is `uint32_t`) | ||
* | ||
* The smallest type which contains all values of both types is | ||
* `int64_t`. | ||
*/ | ||
using ErrorNumber = int64_t; |
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.
Does this really make sense? I imagine that the same error code will mean something totally different depending on whether it's a Posix error code or a Windows one. So what are the cases where it makes sense to introspect the errNo
of an arbitrary SysError
instance (without knowing whether it's a Posix
or Windows
one?
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.
You right, there will be some now-incorrect (on windows) code inspecting this. Either we need to added a bunch of OS-conditional constants for the comparisons, or we need to push the number into each subclass and create a bunch of virtual methods.
I can do one of those two up-front, but we could also just file an issue 😅. It won't make the Unix version less correct, and I'll be focused on just getting Windows to build at all for the next few PRs before worrying about Windows working correctly.
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.
This shouldn't have to be exposed on the superclass of the platform-specific errors.
If platform-generic code needs to know something based on those codes, I don't think that happens very often, so you could add a method for it, and implement it for both platforms.
Alternatively, just catch PosixError in platform-generic code. I'd rather have stupid implementations than stupid interfaces.
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.
I'd rather have stupid implementations than stupid interfaces.
OK yeah that's a very fair point :)
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.
OK the field is now just on PosixError
, and the catch blocks that not throw certain errors are catch PosixError
rather than catch SysError
.
30d33a6
to
1a9778f
Compare
SysError
, PosixError
, WinError
, NativeError
PosixError
from SysError
0da2e75
to
2b4d5ab
Compare
This is way too much code churn (especially if it's a refactoring for a platform we may never end up supporting). It seems better to me to keep |
2b4d5ab
to
f467240
Compare
PosixError
from SysError
SystemError
from SysError
Did the rename @edolstra requested above and in the meeting. |
Most of this is a `catch SysError` -> `catch SystemError` sed. This is a rather pure-churn change I would like to get out of the way. **The intersting part is `src/libutil/error.hh`.** On Unix, we will only throw the `SysError` concrete class, which has the same constructors that `SystemError` used to have. On Windows, we will throw `WinError` *and* `SysError`. `WinError` (which will be created in a later PR), will use a `DWORD` instead of `int` error value, and `GetLastError()`, which is the Windows equivalent of the `errno` machinery. Windows will *also* use `SysError` because Window's "libc" (MSVCRT) implements the POSIX interface, and we use it too. As the docs describe, while we *throw* one of the 3 choices above (2 concrete classes or the alias), we should always *catch* `SystemError`. This ensures no matter how the implementation changes for Windows (e.g. between `SysError` and `WinError`) the catching logic stays the same and stays correct. Co-Authored-By volth <volth@volth.com> Co-Authored-By Eugene Butler <eugene@eugene4.com>
f467240
to
6208ca7
Compare
Motivation
Most of this is a
catch SysError
->catch SystemError
sed. This is a rather pure-churn change I would like to get out of the way. The intersting part issrc/libutil/error.hh
.On Unix, we will only throw the
SysError
concrete class, which has the same constructors thatSystemError
used to have.On Windows, we will throw
WinError
andSysError
.WinError
(which will be created in a later PR), will use aDWORD
instead ofint
error value, andGetLastError()
, which is the Windows equivalent of theerrno
machinery. Windows will also useSysError
because Window's "libc" (MSVCRT) implements the POSIX interface, and we use it too.As the docs describe, while we throw one of the 3 choices above (2 concrete classes or the alias), we should always catch
SystemError
. This ensures no matter how the implementation changes for Windows (e.g. betweenSysError
andWinError
) the catching logic stays the same and stays correctContext
#8901 will take advantage of this.
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.
CC @wegank