[rrd-developers] rrdtool 1.4 development ready to go

[Florian Forster]

Hi José,

On Sun, Sep 07, 2008 at 09:03:41PM +0200, José Luis Tallón wrote:
> >> The strategy we used was to re-design everything in C++,
> >> object-oriented, and then code the resulting "projection" onto C.

I like your general approach, especially the object oriented approach
which fits nicely for librrd.

Of course, as you predicted, not everybody likes everything about it, so
here I go: ;)

> The handle types are typedef'd (void*) pointers,

I much prefer to use `real' opaque handles. Something like:
-- 8< --
 struct rrd_s;
 typedef struct rrd_s rrd_t;

 rrd_t *rrd_open (const char *file);
-- >8 --

I prefer that, because void-pointers can be assigned to anything, so
 whatever_t *foo = (rrdng_rrd_t) bar;
doesn't result in an error. Also, if you define `struct rrd_s' in an
internal header, no casting is required internally, because the pointer
is the right type to begin with.

Last but not least, I think typedefs to pointers are confusing and make
the code hard(er) to read..


> The variadic interfaces (stdarg-based) are provided for programmer's
> convenience: when we already have all parameters, it is easier to just
> make one call with all required data and have the RRD created than
> making, say, 10 calls to fill in all attributes.

This kind of sabotages the advantage you gain by replacing the argv-
interface. To set many aspects at once, I'd rather use a pointer to a
struct, like the `hints' passed to `getaddrinfo(3)'. For example:
-- 8< --
 #define RRD_SETTINGS_INIT { 300, time (NULL) - 10, ... }

 rrd_settings_t settings = RRD_SETTINGS_INIT;

 settings.step = 30;
 /* settings.start: Default set by initialization macro. */
 ...

 status = rrd_create (filename, &settings,
   (sizeof (ds) / sizeof (*ds)), ds,
   (sizeof (rra) / sizeof (*rra)), ds);
-- >8 --

Right now, this interface isn't really necessary yet: There are two
options to `rrd_create' right now, that's easy enough. But if new
options are added this approach is *much* easier to extend without
breaking existing code. In the example above, imagine a new option /
member is added to `rrd_settings_t' - if the code isn't changed, the
default will be used. Without touching one line of code!

The rest of the interface should use the same mechanism: Put all the
*optional* arguments in a struct, but pass *required* arguments
directly.


My last point, and here I really dive into personal preference without
any good arguments for it: Mixing names such as `librrd_version' with
``smallCapsNames'' is horrible. I don't like the caps approach much, so
I'd much prefer `rrd_get_ds_count' over `librrd_countDSs'. Actually, I'd
simply change that function to:
 int rrd_get_ds (rrd_t *, size_t *ds_count_ret, rrd_ds_t * const *ds_ret);
(Hope I didn't get the `const' wrong..)


I've done a quick mockup, how I'd design the API. I didn't even try to
compile anything, so there may be syntax errors and stuff all around.
It's just to get the basic idea(s) across. Any ideas / comments?

Regards,
-octo
-- 
Florian octo Forster
Hacker in training
GnuPG: 0x91523C3D
http://verplant.org/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rrd.h
Type: text/x-chdr
Size: 3080 bytes
Desc: not available
Url : http://lists.oetiker.ch/pipermail/rrd-developers/attachments/20080908/acb89b64/attachment-0002.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.oetiker.ch/pipermail/rrd-developers/attachments/20080908/acb89b64/attachment-0003.bin