gracinet added inline comments. INLINE COMMENTS
> kevincox wrote in nodemap.rs:158 > This strikes me as a weird name. The fact that it is an adjective rather than > a noun is a hint. Can you rename to answer "Growable what?" In a previous version, I was calling it `mutable` (also an adjective, btw), but that became just wrong with the introduction of the separate root block. I don't have many ideas: - `added_inner_blocks` ? actually, these are added on top of the readonly part and often mask some of the readonly blocks - `added_non_root_blocks` ? Eww - `mutable_inner_blocks` ? I'm no native speaker, but the adjective thing doesn't deter me, I would expect it to be rather clear that it refers to blocks, given the type. Open to suggestions, leaving as is for the time being > kevincox wrote in nodemap.rs:249 > I would use > https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.debug_struct > for consistency unless you really want to avoid printing the struct name. sadly, `Formatter.debug_struct` doesn't tale `?Sized` arguments (at least in our target 1.34 version): --> hg-core/src/revlog/nodemap.rs:298:32 | 298 | .field("readonly", readonly) | ^^^^^^^^ doesn't have a size known at compile-time | = help: the trait `std::marker::Sized` is not implemented for `[revlog::nodemap::Block]` Kept it as it was for the time being, just dropped the unneeded anonymous lifetime Omitting the struct name was indeed on purpose, to make it more manageable in test data. That was useful in early versions. I would have been willing to drop it for the final form. Anyone using this for real data wouldn't care about just a few bytes at the beginning. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7793/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7793 To: gracinet, #hg-reviewers, kevincox Cc: durin42, kevincox, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
