[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: libphonon and kdebase4-runtime: Code advice/review
From:       Michael Pyne <mpyne () kde ! org>
Date:       2009-09-28 22:58:52
Message-ID: 200909281859.00937.mpyne () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Monday 28 September 2009 09:47:08 Colin Guthrie wrote:
> > Well you could still give the default implementation, but in the .cpp
> > file as opposed to the .h.  (having it inline in the .h file means that
> > the compiler may insert it like a macro into calling code instead of
> > making the function call.  If you later change the function call
> > implementation then programs compiled against the older header would not
> > pick up the change, which can lead to tricky issues).
> 
> There is no default .cpp file here: it's an abstract interface. It's up
> to all the backends to implement it themselves, so there would be no
> default implementation I could make except in the header file. If there
> had been a .cpp file for this, I would have put it there.
> 
> Have I missed something or was this just a general "it should go in the
> cpp file" type code review answer? (I agree totally if it is!! :p)

You can add a .cpp file if necessary and just make sure it's referenced by 
CMakeLists.txt.

> >>> Exported, yes. But I would only expect it to be used by the Phonon KCM
> >>> (although if we export it we need to be ready for any Phonon-using app
> >>> to get a hold of it ;)
> >>
> >> Yeah true, but I guess that isn't too bad overall considering it's a
> >> fairly simple API. Is there some other trick we could do to make this
> >> harder? Or perhaps just by copying the header but not the full
> >> implementation? (although that latter option feels ugly too!)
> >
> > No need to put the clamps down, client code that really cares about it
> > will just edit the phononrc on disk or various other sundry hacks.
> 
> Well this is really what I'm trying to avoid. While the config file will
> be used in some cases, what I'm actually implementing at the backend is
> an alternative to the config file used when the audio backend ultimately
> uses pulseaudio. So the main design goal from my perspective here is to
> make sure client code is totally unaware of any config file at all...
> i.e. it must have an API to access, query and change the config, never
> access it directly via a file.

All I mean is that there's no reason to put a lot of time into trying to keep 
clients from abusing the exported class and yet allow the KCM to work.  If 
it's an internal API for instance but must be public for this reason we just 
document it as such in the APIDOX for the methods involved. i.e. @internal

Another evil thing to do is something like our virtual_hook(int method, void* 
data) but don't document how to use it. ;)  (When we mean for client code to 
call a virtual_hook we provide wrapper functions to marshal/demarshal 
appropriately).

But there's no reason to go this route as long as the public API can be used 
as documented (which can include documented as "do not use unless you are 
kcm_phonon")

Regards,
 - Michael Pyne

["signature.asc" (application/pgp-signature)]

>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<


[prev in list] [next in list] [prev in thread] [next in thread]