Is this as dangerous as I think it is?

[Nate Finch]

I don't really have a problem with the code as-is.  As Roger said, it would
take some effort to define IsAliveDoc to have more capacity than its
length, and therefore get into the problem you're talking about.

However, the simplest answer might just be to make IsAliveDoc into a
DocElem, rather than a []DocElem (bson.D)... then the same code would just
look like this:

var IsAliveDoc = DocElem{"life", Alive}

asserts = append(bson.D{IsAliveDoc}, asserts...)
.. this makes the code much more clear that we won't be adding anything to
the slice that IsAliveDoc is in, and it also makes it more composable if
you have just one or two other asserts:

asserts = bson.D{IsAliveDoc, SomeOtherAssert}

-Nate

On Thu, Oct 22, 2015 at 9:26 AM roger peppe <roger.peppe at canonical.com>
wrote:

> By the language-defined semantics of append, this is fine.
> It would actually be quite hard to define isAliveDoc such
> that it had cap != len.
>
> That said, it would be easy to write:
>
>     // addAsserts returns the result of appending elems to docs.
>     // It does not mutate docs.
>     func addAsserts(docs bson.D, elems ...bson.DocElem) bson.D {
>         var r bson.D
>         r = append(r, base...)
>         return append(r, elems...)
>     }
>
> or similar, which is semantically equivalent and may well compile to
> the same code in the end.
>
>
> On 22 October 2015 at 13:42, John A Meinel <john.meinel at canonical.com>
> wrote:
> > While doing a review, I came across this snippet of code:
> > asserts = append(isAliveDoc, asserts...)
> >
> > It took me a while to understand why the thing that is being appended is
> at
> > the beginning. I realize it probably wants to insert it at 0, but I
> thought
> > it was an object, not a slice, so how could it go at the beginning. But
> > digging shows it is actually a slice:
> >
> > https://github.com/juju/juju/blob/master/state/life.go#L36
> > var isAliveDoc = bson.D{{"life", Alive}}
> >
> > and bson.D:
> > http://bazaar.launchpad.net/+branch/mgo/v2/view/head:/bson/bson.go#L118
> > type D []DocElem
> >
> > Doesn't that mean that every time we call something like this, we are
> > potentially mutating the isAliveDoc statement?
> >
> > I realize it probably "works" because isAliveDoc has capacity 1 and thus
> > must be reallocated everytime you append, and thus we don't end up with
> an
> > isAliveDoc that actually carries arround all the assertions from
> everything
> > else.
> >
> > but that seems very "assume no bad side effects" rather than valid code.
> >
> > Thoughts?
> >
> > John
> > =->
> >
> >
> > --
> > Juju-dev mailing list
> > Juju-dev at lists.ubuntu.com
> > Modify settings or unsubscribe at:
> > https://lists.ubuntu.com/mailman/listinfo/juju-dev
> >
>
> --
> Juju-dev mailing list
> Juju-dev at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20151022/4a403129/attachment.html>