Add useful impls #172
Add useful impls #172
Conversation
| @@ -80,7 +80,7 @@ impl<FileId> Label<FileId> { | |||
|
|
|||
| /// Represents a diagnostic message that can provide information like errors and | |||
| /// warnings to the user. | |||
| #[derive(Clone)] | |||
| #[derive(Clone, Debug, Hash, PartialEq, Eq)] | |||
There was a problem hiding this comment.
This seems a little large to derive Debug on ... can I ask your use case?
There was a problem hiding this comment.
The debug one I can actually remove. My main use case for the impls is salsa. Salsa requires that arguments for a query must implement hash, eq and partialEq
There was a problem hiding this comment.
This seems expensive to be in a salsa query, wouldn't it be faster to just recompute it instead of comparing all the strings to one another?
| @@ -330,7 +330,7 @@ where | |||
| } | |||
|
|
|||
| /// A file that is stored in the database. | |||
| #[derive(Debug, Clone)] | |||
| #[derive(Debug, Clone, Hash, PartialEq, Eq)] | |||
There was a problem hiding this comment.
Does it make sense to compare files like this? Eq will be expensive and I'm not sure how useful it will be.
There was a problem hiding this comment.
Yeah, I'm not sure about having these impls. 🤔
|
Yeah I've realised that some the impls could be removed and that I should
have improved my code instead.
…On Sat, 29 Feb 2020, 21:22 Brendan Zabarauskas, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In codespan/src/file.rs
<#172 (comment)>:
> @@ -330,7 +330,7 @@ where
}
/// A file that is stored in the database.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Hash, PartialEq, Eq)]
Yeah, I'm not sure about having these impls. 🤔
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#172?email_source=notifications&email_token=AEYSL2UXQ4VVWDA77NOFWEDRFF6BBA5CNFSM4K62DQ3KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCXOT3EQ#discussion_r386056447>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYSL2U7OLXXWJSQL6VNJKDRFF6BBANCNFSM4K62DQ3A>
.
|
|
No worries! I see you're also interested in Salsa! Note that I have been working on making it easier to implement custom |
|
Kl I'll hold off with this then |
No description provided.