Hi Florian, On Feb 4, 2010, at 1:20 AM, Florian Forster wrote:
> thanks for your patch :) I think it's kind of weird that JSON doesn't > differentiate between -inf, inf and nan, but since that's what the > standard says, that's what we should be doing. My pleasure, and thank you for a great piece of well-conceived software! > I'm a bit uncomfortable using "isinf" here. We've had a great deal of > trouble with "isnan". Since "isinf" isn't used anywhere else and the C99 > standard says it is a macro, I'll change this to: > > #ifdef isinf > if (isnan (value) || isinf (value)) > #else > if (isnan (value)) > #endif > > Does this sound alright to you? I certainly overlooked the portability aspect of isinf. :-) With your proposed approach, won't we still get invalid json on a platform where isinf doesn't get defined? This doesn't matter in my particular experience (and never will, as on RHEL we'll always end up with isinf being defined), but it could be a gotcha. When invalid JSON is emitted, it's a fairly high-impact problem, since the JSON data is batched and the receiving end must parse the batch in its entirety, the entire batch is lost if even a single nan or inf ends up in the output. Assumption: in system header files, isnan and isinf are defined together -- so we get either both or neither. So with this assumption, seems like the only chance of the problem happening is the scenario when we end up with NAN_ZERO_ZERO (embedded platforms?), and isnan and isinf aren't defined? In this case, your isnan fixup on collectd.h:136 is needed. What about applying a similar fixup for isinf? (ideally needed in collectd-nagios.c, too) --- a/src/collectd.h +++ b/src/collectd.h @@ -135,6 +135,9 @@ typedef bool _Bool; # ifndef isnan # define isnan(f) ((f) != (f)) # endif /* !defined(isnan) */ +# ifndef isinf +# define isinf(f) (!isnan (f) && isnan (f - f)) +# endif /* !defined(isinf) */ #endif /* NAN_ZERO_ZERO */ /* Try really, really hard to determine endianess. Under NexentaStor 1.0.2 this Thanks, - C _______________________________________________ collectd mailing list [email protected] http://mailman.verplant.org/listinfo/collectd
