Separate SystemError from SysError#9737
Conversation
github-actions
bot
added
new-cli
Ericson2314
force-pushed
the
sys-error-split
branch
2 times, most recently
from
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.
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.
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.
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.
I'd rather have stupid implementations than stupid interfaces.
OK yeah that's a very fair point :)
There was a problem hiding this comment.
OK the field is now just on PosixError, and the catch blocks that not throw certain errors are catch PosixError rather than catch SysError.
Ericson2314
force-pushed
the
sys-error-split
branch
2 times, most recently
from
30d33a6 to
1a9778f
Compare
Ericson2314
changed the title
SysError, PosixError, WinError, NativeErrorPosixError from SysError
Ericson2314
force-pushed
the
sys-error-split
branch
3 times, most recently
from
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 |
Ericson2314
force-pushed
the
sys-error-split
branch
from
2b4d5ab to
f467240
Compare
Ericson2314
changed the title
PosixError from SysErrorSystemError 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 <[email protected]> Co-Authored-By Eugene Butler <[email protected]>
Ericson2314
force-pushed
the
sys-error-split
branch
from
f467240 to
6208ca7
Compare
5 participants
Motivation
Most of this is a
catch SysError->catch SystemErrorsed. 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
SysErrorconcrete class, which has the same constructors thatSystemErrorused to have.On Windows, we will throw
WinErrorandSysError.WinError(which will be created in a later PR), will use aDWORDinstead ofinterror value, andGetLastError(), which is the Windows equivalent of theerrnomachinery. Windows will also useSysErrorbecause 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. betweenSysErrorandWinError) 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