Some subsurface notes from a week of diving
dirk at hohndel.org
Mon Mar 17 08:13:54 PDT 2014
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
> 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
> 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.
More information about the subsurface