On Mon, Jan 25, 2016 at 10:38 AM, Barret Rhoden <[email protected]> wrote:
> On Friday, January 22, 2016 at 2:30:37 PM UTC-5, crossd wrote: > > > > The following changes since commit > > 6ae8195b99f28d2f2735dcde2a723a4bde3142ef: > > > > Add taps for pipes. (2016-01-14 16:04:46 -0500) > > > > are available in the git repository at: > > > > [email protected]:dancrossnyc/akaros.git slice > > > From a3cc5dd44b4769b82823a63ce5c4a41de4b45ad5 Mon Sep 17 00:00:00 2001 > > From: Dan Cross <[email protected]> > > Date: Fri, 22 Jan 2016 14:26:11 -0500 > > Subject: Slices: A growable list of pointers. > > > diff --git a/kern/lib/slice.c b/kern/lib/slice.c > > > +int slice_put(struct slice *slice, size_t i, void *p) > > +{ > > + if (i >= slice->len) > > + return 0; > > Minor thing, but can you have success return 0 and failure return > non-zero? (for this function and for all of these in this library). > That's the general style we have for functions that return int. > Alternative, you can have all of these return bool (TRUE/FALSE) for > success. > Done; changed to use bool. (Incidentally, this means that the values didn't actually change, but the intent is much clearer.) > + slice->ptrs[i] = p; > > + return 1; > > +} > > + > > +int slice_del(struct slice *slice, size_t i) > > +{ > > + if (i >= slice->len) > > + return 0; > > + memmove(slice->ptrs + i, slice->ptrs + i + 1, > > + (slice->len - i + 1) * sizeof(void *)); > > I think there's an off-by one (or two!) with the length calculation. > Should it be (slice->len - (i + 1)) or (slice->len - i - 1)? Imagine we > have len = 2 and remove the i = 0 item. The original calculated len was > 2 - 0 + 1 = 3. It should be 1. Another way to look at it is the +1 was > due to the count of all items up to and including i, and that should be > subtracted from the total length. > You are correct; (i + 1) should have been parenthesized. > +void *slice_finalize(struct slice *slice) > > Should this be a void ** for the return type? I wasn't quite clear on > what finalize does. (remove the list from the slice, return it, and the > caller needs to know how long it is?) > We talked about this; it wasn't clear to me that it was exactly legal, but upon closer reading of the standard, I think that 'void **' is ok. Changed accordingly. > +{ > > + void **ps; > > + > > + ps = kreallocarray(slice->ptrs, slice->len, sizeof(void *), > KMALLOC_WAIT); > > Is the intent here to shrink the existing allocation? Our realloc > doesn't actually shrink; it will only grow. > Talked about this face-to-face. Leaving as is for now (our realloc may shrink at some point). > + assert(ps != NULL); > > + slice->len = 0; > > + slice->capacity = 0; > > + slice->ptrs = NULL; > > + > > + return ps; > > +} > > Barret > > -- > You received this message because you are subscribed to the Google Groups > "Akaros" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
