Review Request: wetter.com Weather Ion

Thilo-Alexander Ginkel thilo at ginkel.com
Tue Nov 3 18:51:47 GMT 2009


On Tuesday 03 November 2009 18:51:05 Aaron Seigo wrote:
> > On 2009-11-03 17:00:51, Will Stephenson wrote:
> > > /trunk/kdereview/plasma/dataengines/weather/ions/ion_wettercom.cpp,
> > > line 44
> > > <http://reviewboard.kde.org/r/2026/diff/2/?file=13614#file13614line44>
> > >
> > >     By convention, public member variables of pimpl classes are not
> > > names with m_ prefixes - since accessing them via d-> in the outer
> > > class makes it obvious that they belong  to the pimpl
> 
> this isn't public API. why does it have a pimpl at all? i think some people
>  have missed the point of the FooPrivate classes: maintaining binary
>  compatibility in publicly exported APIs. for anything else they are just a
>  bit more overhead, one more level of indirection and one more opportunity
>  for a memory leak.

I chose this approach as that's what the existing Weather Ions are using, but 
also agree that this is not a public API, so binary compatibility is a non-
issue. If you prefer, I can easily remove the pimpl.

Regards,
Thilo



More information about the kde-core-devel mailing list