Partialeq#206
Conversation
AlexandruCihodaru
requested review from
alexandruag,
bonzini and
jiangliu
as code owners
AlexandruCihodaru
force-pushed
the
partialeq
branch
from
b576045 to
e5f3851
Compare
Signed-off-by: Alexandru Cihodaru <alexandru.ciprian.cihodaru@intel.com>
Implement PartialEq for Error enum and update tests. Signed-off-by: Alexandru Cihodaru <alexandru.ciprian.cihodaru@intel.com>
Implemented PartialEq for Error enum. Considering that from this file the only io error returned are OS error usage of raw_os_error offers anything that is needed. Updated tests to use PartialEq for error cases. Signed-off-by: Alexandru Cihodaru <alexandru.ciprian.cihodaru@intel.com>
Derive PartialEq and update tests to check for equality on error cases. Signed-off-by: Alexandru Cihodaru <alexandru.ciprian.cihodaru@intel.com>
AlexandruCihodaru
force-pushed
the
partialeq
branch
from
e5f3851 to
0ac2b35
Compare
| }, | ||
| ) => left_expected == right_expected && left_completed == right_completed, | ||
| (Error::IOError(left), Error::IOError(right)) => { | ||
| // error.kind should be enough to assert equallity because each error |
There was a problem hiding this comment.
While this is true, it does not mean that kind can be used to compare errors. The reason why io::Error does not have PartialEq implemented is because it cannot be implemented truthfully. The first example that comes to mind is NotFound. A not found error thrown for file1 is not equal to an error thrown for file2. This is one of the reasons why we cannot actually compare the io::Error like that because it has other metadata that is specific to the error type. We can say instead that we are willing to accept the loss of information in comparing errors because vm-memory errors are widely used in other crates as well and it is very limiting to not have PartialEq implemented.
|
|
||
| impl PartialEq for Error { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| match (self, other) { |
There was a problem hiding this comment.
One reason I don't particularly like these kinds of implementations for PartialEq is that it will silently fail when you add a new error type if you don't explicitly add it in the below match as well. I would propose we add a test that just fails when a new variant is added and has a comment something like: "If this failed it means you added a new error type. Make sure you also add that to the PartialEq implementation. Once you've done that feel free to fix this test". This will serve as a warning when adding new types. I am also open to other ideas as this is rather dummy 😆
|
Given the issues that @andreeaflorescu mentioned, I'd rather add an |
Agreed. We've actually replaced all the As for all the assertions on .is_err()/is_ok(), I suspect a lot of them we dont really care about matching the actual error, and just replacing it with .unwrap()/.unwrap_err() is fine (and produces significantly better error messages in case of failure). There's a clippy lint against these as well (clippy::assertion_on_result_states) - I'll open an issue for converting these, in case someone is really bored one day (Edit: #337) |
4 participants
Summary of the PR
Implement PartialEq for Error enums.
Fixes: #194
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
unsafecode is properly documented.