On Mon, Jun 22, 2015 at 11:09:19PM +0200, Robert C. Helling wrote: > Hi Rick, > > > On 21 Jun 2015, at 03:13, Dirk Hohndel <[email protected]> wrote: > > > > Robert, I consider you the maintainer of the planner. I took Rick's rather > > straight forward looking patches earlier, but would you take a look at > > this one before I apply it? > > I am currently looking at the patch and write down things I notice. > > The gas change looks good for our case of a switch at a depth without a stop > (even when mandatory stop time is set to 0). But when I turn on displayed > transitions I would expect the gas change to be shown on the stop rather than > on the upwards transition that leads to it (as the later is only the place to > print it if there is no stop for it). I attached a patch (to be applied on > top of yours or for yours to be amended with it) to fix that. > > > > My compiler gives a warning that in that very long if statement there is || > next to && and thus some parenthesis might add clarity. I would like to chime > in here and also suggest to break that condition over several more lines > essentially with one part of the || statement per line and to add comments > there so we will not be as confused again when we look at this in a few > months. > > Another stylistic thing: The bool gaschange is not a good name anymore and > should be renamed to something like gaschange_after and a similar > abbreviation gaschange_before could be introduced. Here is a second patch for > that: > > > > But apart from that, this finally seems to do the right thing. Thanks a lot > for sorting this out!
Thanks to both of you for working on this. Because of some of the code cleanup that I did over the weekend these were kinda hard to apply. I tried to close a few dozen Coverity issues and ended up changing some of those very same conditional statements since Coverity convinced me that it was possible for nextdp to be NULL... Please take a close look at what I just pushed to make sure I didn't end up breaking things now that you finally got them right. Thanks /D _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
