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? > 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. Bryce _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
