58 separate db format - #59
Conversation
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use pretty_assertions::assert_eq; |
There was a problem hiding this comment.
This is a pretty neat library for tests that will color the parts that are different when assert_eq fails.
There was a problem hiding this comment.
Neat indeed! Should we use it in other tests too?
There was a problem hiding this comment.
There is no big downside to using it. but I usually only use it when a test fails and I have a hard time figuring out why.
In this case the serialization test was failing because the comments map sometimes serialized in different orders. it was hard to see what was different so I added the pretty_assertion.
merces
left a comment
merces
left a comment
There was a problem hiding this comment.
Thank you! I left a few questions/thoughts for further discussion, but none of them is a blocker. Will merge it soon. :)
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use pretty_assertions::assert_eq; |
There was a problem hiding this comment.
Neat indeed! Should we use it in other tests too?
| // to file offset passed to goto() | ||
| pub comment_name_list: Vec<DbComment>, | ||
|
|
||
| // TODO: comments and comment_name_list are redundant, we should only store one of them |
There was a problem hiding this comment.
To be honest, I can't remember why I decided to store both of them. I think my line of thought was that the user could add a name to an offset so this name could be shown when code references it. For example:
jmp 0x401000 could be shown as jmp destination if the user set the name destination to 0x401000, but this only makes sense in a disassembly view, which we don't have yet. Also, even when names and comments are two different vectors, we can still show the elements from both of them in a single Names window, but store them separately in the database and build the comment_name_list from comments and names in the future. Alternatively, perhaps we can get rid of comment_name_list entirely and build this list when the Names window is shown? Just thoughts. :)
There was a problem hiding this comment.
I think it makes sense to store just one of them and derive the data of the other one.
I will make a PR the "deprecates" the comment_name_list from the database.
The idea is that we stop reading from it but we still write to it. That way at least for a while the current version can still create DBs for previous versions. then after a few releases we remove the field for good.
(I am probably overly paranoid with this "forward" and "backwards" compatability stuff)
There was a problem hiding this comment.
Created the MR for it:
#63
I decide to only make it "backwards" compatible, not forward compatible. I explain it in the other MR.
| pub comment_name_list: Vec<DbComment>, | ||
|
|
||
| // TODO: comments and comment_name_list are redundant, we should only store one of them | ||
| // but to avoid breaking existing .dz6 files, we will keep both for now |
There was a problem hiding this comment.
Thanks for thinking about it! Currently, there's no way an user can create a name, so maybe in the future we can build this list (for the Names window) from comments without breaking anything by ignoring comment_names_list coming from databases created by earlier versions of dz6?
2 participants
Solves #58