Closed by commit rHGc3cc881aaa17: rust-nodemap: input/output primitives
(authored by gracinet).
gracinet marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state "Needs
Review".
R
Alphare updated this revision to Diff 20281.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D7796?vs=20235&id=20281
BRANCH
default
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7796/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D77
kevincox added inline comments.
kevincox accepted this revision.
INLINE COMMENTS
> nodemap.rs:191
> fn new() -> Self {
> -Block([-1; 16])
> +Block([255u8; BLOCK_SIZE])
> }
A comment why 255 is a good number would be nice.
> nodemap.rs:334
> +// Assert that
Alphare updated this revision to Diff 20235.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D7796?vs=20215&id=20235
BRANCH
default
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7796/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D77
kevincox added inline comments.
INLINE COMMENTS
> Alphare wrote in nodemap.rs:268
> I think that would be sufficient indeed. How about making `Block` into a
> `[u8; 64]`? It will be less expressive, but this will no longer be needed.
I like that. It makes it much more explicit that it is a back
Alphare added inline comments.
INLINE COMMENTS
> kevincox wrote in nodemap.rs:268
> This doesn't address the primary problem of padding. There is nothing in the
> code that guarantees that there will be no padding inserted into `Block`
> (although it is unlikely to ever happen). This is because
yuja added a comment.
> +) -> (Box + Send>, Vec) {
> +let (readonly, vec) = self.into_readonly_and_added();
> +// Prevent running `v`'s destructor so we are in complete control
> +// of the allocation.
> +let vec = mem::ManuallyDrop::new(vec);
> +
> +) -> (Box + Send>, Vec) {
> +let (readonly, vec) = self.into_readonly_and_added();
> +// Prevent running `v`'s destructor so we are in complete control
> +// of the allocation.
> +let vec = mem::ManuallyDrop::new(vec);
> +
> +let bytes = unsafe {
> +
kevincox added inline comments.
INLINE COMMENTS
> Alphare wrote in nodemap.rs:268
> @kevincox @durin42: I've changed the code to use the type-level
> `ManuallyDrop` instead of `mem::forget`, added the `transmute` to be extra
> paranoid and added a comment.
This doesn't address the primary prob
Alphare updated this revision to Diff 20215.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D7796?vs=20025&id=20215
BRANCH
default
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7796/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D77
Alphare added a comment.
@kevincox To re-iterate, I've taken over this series, so excuse any
additional inaccuracies.
INLINE COMMENTS
> gracinet wrote in nodemap.rs:268
> About a method to output to a writer: for the time being, we're avoiding
> doing I/O directly in Rust because most of it
This revision now requires changes to proceed.
durin42 added a comment.
durin42 requested changes to this revision.
I'm just not comfortable with the `unsafe` block here, especially with the
comments from @kevincox . That said, if the `unsafe` can't disappear, it
probably needs a pretty thoro
marmoute updated this revision to Diff 20025.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D7796?vs=19636&id=20025
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7796/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7796
AFFECTED FILE
marmoute added a comment.
I don't see anything wrong in this changeset, but there have been a lots of
exchange between @kevincox and @gracinet. @kevincox Is there still code you are
unconfortable with?
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D77
gracinet added inline comments.
gracinet marked 2 inline comments as done.
INLINE COMMENTS
> kevincox wrote in nodemap.rs:268
> I thought about this more and I think I am okay doing it this way. It seems
> like this should be well defined as long as there is no padding. However on
> that note I
gracinet edited the summary of this revision.
gracinet updated this revision to Diff 19636.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D7796?vs=19329&id=19636
BRANCH
default
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7796/new/
REVIS
kevincox added a comment.
Maybe what we should do is keep the test I proposed, but change the
definition of `BLOCK_SIZE` to be based upon the number of contained `i32`
instead of `size_of`.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7796/new/
REV
kevincox added a comment.
Ah yes. So my test would be ineffective.
The problem that IIUC padding is not guaranteed to be initialized, and in
rust all values need to be initialized. So if you cast a type with padding to a
`u8` it is undefined behaviour.
So we would need a test that s
gracinet added a comment.
@kevincox (replying hastily because I'm also finishing something unrelated
today).
Doesn't `mem::size_of` guarantee to take any padding into account? At least
that's what the doc seems to say:
https://doc.rust-lang.org/std/mem/fn.size_of.html
REPOSITORY
rHG Mer
kevincox added inline comments.
INLINE COMMENTS
> kevincox wrote in nodemap.rs:268
> This makes me uncomfortable. There is really no guarantee that there is no
> padding around `Block` or `RawElement`. If there is any padding this is UB. I
> would much prefer one of:
>
> 1. Store growable as a
This revision now requires changes to proceed.
kevincox added inline comments.
kevincox requested changes to this revision.
INLINE COMMENTS
> nodemap.rs:268
> +Vec::from_raw_parts(
> +vec.as_ptr() as *mut u8,
> +vec.len() * BLOCK_SIZE,
This makes me un
gracinet updated this revision to Diff 19329.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D7796?vs=19137&id=19329
BRANCH
default
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7796/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7
gracinet updated this revision to Diff 19137.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D7796?vs=19045&id=19137
BRANCH
default
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7796/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7
gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
These allow to initiate a `NodeTree` from an immutable opaque
sequence of bytes, which could be passed over from Python
(extracted from a `PyB
24 matches
Mail list logo
