On Thu, 19 Mar 2015 11:29:52 -0700 Bryce Harrington <[email protected]> wrote:
> On Thu, Mar 19, 2015 at 11:55:06AM +0200, Pekka Paalanen wrote: > > On Wed, 18 Mar 2015 10:58:37 -0700 > > Bryce Harrington <[email protected]> wrote: > > > > > On Wed, Mar 18, 2015 at 03:27:21PM +0200, Pekka Paalanen wrote: > > > > From: Pekka Paalanen <[email protected]> > > > > > > > If 0 time is returned, are we certain that's not going to screw up > > > callers in some unexpected way? > > > > We don't care too much. Reading the current time is always > > somewhat uncertain and the calling code should be implicitly already > > dealing with surprising values. Like in the follow-up I have a sanity > > check to not delay for more than a second in the worst case. > > > > > If it's never meant to fail, and we don't want to propagate error codes > > > from here, why not assert()? > > > > It's not an assert() because there is a good chance it might not be a > > bug in Weston's own code. If Weston is used in production, logging the > > error and having a glitch is IMHO better than outright dying. > > The man page of assert() indicates that for non-debug builds (NDEBUG not > defined), the assert() macro does nothing. Aside from logging an error > message, it seems to do what you want? Non-debug means NDEBUG is defined - does anyone actually use that? It prevents running our test suite, too. > > I think we are missing a bit of infrastructure here: an assert-like > > macro that in debug builds aborts, but in production builds just logs > > the error with as much detail as possible and attempts to continue. > > It'd be good to also include rate limiting of logging. > > Yes, I'd love to see this, I think it'd solve the need nicely. > > > > Otherwise, seems to me like there'd be little harm propagating the error > > > by returning false. The current callers do ignore clock_gettime's > > > return, but maybe some test someday will want to test conditions where > > > clock_gettime wouldn't work properly... > > > > Easy enough to change the return type when such a user appears. Until > > then I don't see the point. > > > > > > > > Apart from the error handling, rest looks fine: > > > > > > Reviewed-by: Bryce Harrington <[email protected]> > > > > Is my explanation satisfactory enough? > > Welll, it didn't change my mind. :-) But the issue is pretty minor, and > not worth blocking on if you want to land it as-is. Ok, cool. If we get the infra, it's an easy grepping for 'warned' or even weston_log and assert to find the places that will benefit. Thanks, pq _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
