Some subsurface notes from a week of diving

[Dirk Hohndel]

On Sun, 2014-03-16 at 15:52 -0700, Linus Torvalds wrote:
> On Sun, Mar 16, 2014 at 1:57 PM, Dirk Hohndel <dirk at hohndel.org> wrote:
> >
> > Oops. We used to have code that fixes that. Wonder where that got lost.
> 
> I think the new profile lost it in the re-implementation. The old code
> didn't blindly just update the dc number.
> 
> Blindly updating the dc number is wrong for other reasons too. Even if
> we do modulus arithmetic when looking up the dive computer (so that
> "--" and "++" just does the right thing), the overflow in the type
> (which is just a char) would make the modulus come out wrong when it
> overflows. It also makes it impossible to reset sanely when switching
> dives (unless you just *always* reset to zero when switching dives,
> but it's actually nice to walk dives and see your "secondary" dive
> computer the whole time if you have a consistent ordering, without
> having to switch dives and then always switch to the secondary
> explicitly).
> 
> But even if that code is made to do the proper modulus at update, we
> *also* have to be careful when we've switched dives, and the new dive
> has fewer dive computers.
> 
> So I think we need to do both, just to be safe: don't blindly update
> dc numbers, but on use we need to also check the dive number against
> the number of dive computers and reset it to zero if it's beyond the
> end (and there we don't want to do modulus, we really want to just say
> "out or range means 'reset to first'").

I think that's what we tried to do before already - except it didn't
quite do it correctly.

> The attached patch does something like that. I haven't tested it
> extensively. And I changed the calling convention of "select_dc()" to
> take the "struct dive" instead: that's what every single caller
> wanted, and that way it matches (and can just use) the very similar
> "get_dive_dc()".
> 
> So the new rules are:
>  - left/right cursor needs to do the proper modulo arithmetic
>  - number_of_computers() returns the obvious value, except that if
> "dive" is NULL, it still returns 1 so that you can do the modulo thing
> without worrying about divide-by-zero even if there is no current dive
>  - get_dive_dc() (and thus "current_dc") will just return the primary
> dc if the index is out of range
>  - select_dc() does the same thing, but additionally resets dc_number
> to zero for the out-of-range case.
> 
> Dirk, consider this signed-off-by-me, but it might want some more
> testing. I tested it very minimally.

You attached it which makes commenting on the code a little harder...

Look at this little gem. I think I'll reorder this a little bit:

+static inline int number_of_computers(struct dive *dive)
+{
+	int total_number = 0;
+	struct divecomputer *dc = &dive->dc;
+
+	if (!dive)
+		return 1;

(hint: think what happens if dive == null)
Other than that I like it and will use it.

/D