[rrd-developers] rrdtool 1.4 development ready to go

[José Luis Tallón]

Florian Forster wrote:
>> 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..
>   
My opinion is the precisely the opposite in this case, but of course,
just my opinion
.
The --arguably clunky-- typedef is done precisely to avoid pointers as
return types. With the typedefs, you convey a "handle" meaning, just
like it was an integer.

You are right, however, in pointing that it truly defeats type safety
... considering C is a (relatively) loosely typed language, not that
much is lost by this approach. We were looking to hide pointers as much
as possible and hence the tradeoff. As always, your mileage will vary :-)
>> 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 --
>   
The variadic interface internally behaves in this way. The actual
creation routine uses structures to pass information around.
> 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!
>   
I quite don't like macro initializers, but that is just me probably
(used to have bad experiences with pthread/mutex initializers )
Your approach is consistent, even though it creates yet another type
without actual meaning (as seen from the modelling perspective). Not
that this is a problem, anyway ...
> 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..)
>   
That is a const pointer to vector of rrd_ds_t, if I read it correctly...
so it looks good.

The "mixed case" you don't like was actually intentional. My apologies
for not documenting that:
* "librrd_"  is just the prefix, to introduce a namespace of sorts for
librrd and avoid cluttering the global one
* the following component just follows the convention:
 - attributes begin with lowercase, use "sturdyCaps", and gramatically
look like nouns: countDS
 - operations begin with UpperCase:
* the routines which do not provide any semantic, just all lowercase
(i.e. the _version)

> 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?
>   
Thank you for your input :-)


    J.L.