Re: [pupnp-devel] Adding string accessors for UpnpString objects ?

[Marcelo R. J. <mar...@gm...>]

Nick,

On Wed, Mar 3, 2010 at 20:48, Nick Leverton <ni...@le...> wrote:
> I'm working on updating libupnp in Debian and I hope that I can use
> 1.8.x as the mainstream soon.  As part of this, I've done trial ports
> of each Debian libupnp application to pupnp 1.8.  I'm very impressed
> with the way that 1.8.x has hidden the implementation of internal Upnp*
> objects by providing the _get_ accessors.

The basic idea behind of the (yet unreleased) 1.8 series is to hide
the library's internal data structures. That way, the developers would
be free to implement the necessary changes without creating API
incompatibilities.

> One thing I've found is that almost every 1.6.x application directly
> addresses the const char* strings in "struct Upnp*" objects, mostly
> for strcmp purposes.  Now that these are either UpnpString (mostly)
> or DOMString (a few) objects, it should be much safer.
>
> To make it easier to use these _get_ accessors for string objects,
> as well as to hide the implementation even further, I'd like to add
> a new series of accessor functions to the Upnp API.  By using them to
> retrieve string values, a caller doesn't need to care about what sort
> of string-like object has been used.  So instead of saying
>
>   strcmp(Upnp_get_String(UpnpSubscriptionRequest_get_UDN(sr_event)), gateUDN)
>
> an application can use the rather shorter idiom
>
>   strcmp(UpnpSubscriptionRequest_strget_UDN(sr_event), gateUDN)

I can agree with the fact that it does simplify the code, and I am
willing to commit such accessor functions, though I don't agree with
the statement that it hides implementation even more.

> This saves quite a lot of code in some applications: 43 instances in
> linux-igd for instance when it is ported to pupnp 1.8, 40 instances in
> djmount, 28 in gmediaserver.  It's much easier for an application author
> to just use _strget_ whenever they need a string than to remember to wrap
> the relevant _get_ call in Upnp_get_String.  I've done the same for the
> DOMString objects in the interface too, so that their implementation is
> also even better hidden.

I have not touched DOMStrings for two reasons, and in fact, I have not
made up my mind about it and they were the reason why the release of
the 1.8 series was delayed in first place. DOMStrings come from the
ixml library. I did not want at this stage to break the compatibility
with ixml, though I had plans to do so. Along these lines, DOMStrings
would become UpnpString like objects.

On the other hand, DOMStrings are nothing but "char *" most of the
time they are "const DOMString", which means "const char *". There
would be not much gain in hiding them behind an opaque object like
UpnpStrings, because they should not be subject to string
manipulations like concatenation.

So, the central idea to pursue is that libUPnP should export only
UpnpStrings in its interface, thus hiding DOMStrings from the
universe. DOMStrings will only be needed in the IXML interface. As
IXML is not a full DOM Level 3 implementation, and more than that, it
does not want to be such a bloated beast because of the embedded
world, I don't see much gain in making DOMStrings opaque. But still, I
am open to be convinced of the opposite if you have a nice argument.

> This is probably better done before 1.8.x is released, so that the API
> can be stable.  What does the team think, please ?

I agree with your proposal, I just do not agree with the names you
gave, they kinda broke the get/set scheeme, but your idea is ok to me.

> The list of new API calls follows.  I have full patches, will attach
> them later if you think it's worthwhile.

I got your patch and I will do the necessary changes. Thank you very
much for your effort, it is good to know that someone actually
understood what has been done.

Best regards,
Marcelo.