Review Request: Change KCalendarSystem to a Shared D Pointer model

[Pino Toscano]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2557/#review3679
-----------------------------------------------------------

Ship it!


Looks good.


trunk/KDE/kdelibs/kdecore/date/kcalendarsystemhijri.cpp
<http://reviewboard.kde.org/r/2557/#comment3037>

    No need to delete what you don't use, right? ;)



trunk/KDE/kdelibs/kdecore/date/kcalendarsystemhijri_p.h
<http://reviewboard.kde.org/r/2557/#comment3036>

    KCalendarSystemHijri looks not exported nor directly available to the world, so I guess its members can be manipulated freely.


- Pino


On 2010-01-11 17:34:26, John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2557/
> -----------------------------------------------------------
> 
> (Updated 2010-01-11 17:34:26)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> KCalendarSystem is an abstract virtual base class with each calendar system re-implementing virtual methods as required.  Some new features will require new virtual methods to be implemented, but KCalendarSystem has no reserved virtual table slots, doesn't inherit from QObject, and has no virtual_hook.  The only sensible option left is to use the Shared D Pointer method as outlined at http://techbase.kde.org/Policies/Library_Code_Policy#Shared_D-Pointers and have the new 'virtual' methods be implemented in the derived private class.
> 
> There are two non-standard steps involved in doing this that are the main aim of this review:
> 
> 1) The existing base class d is Private, and BC prevents it changing to Protected or adding a new Protected d_ptr.  Instead all the implementation classes are made Private friends of the base class.  As the d is the only private part of the base class I don't see this as an issue.
> 
> 2) The existing derived implementation classes all have existing d pointer Private classes.  Rather than create a second Private class for each implementation, I have simply derived each of the existing Private classes from the base Private class.  I don't think this will break BC.
> 
> This is illustrated in the diff using the base KCalendarSystem class and the derived KCalendarSystemHijri class (Other files have been excluded from the diff for clarity as it's the same pattern repeated).
> 
> Note that the actual calendar implementations are all private, i.e. not exported and headers not installed, only base class KCalendarSystem has ever been exposed.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdecore/date/kcalendarsystemhijri.cpp 1072250 
>   trunk/KDE/kdelibs/kdecore/date/kcalendarsystemhijri_p.h 1072250 
>   trunk/KDE/kdelibs/kdecore/date/kcalendarsystemprivate_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdecore/date/kcalendarsystem.h 1072250 
>   trunk/KDE/kdelibs/kdecore/date/kcalendarsystem.cpp 1072250 
> 
> Diff: http://reviewboard.kde.org/r/2557/diff
> 
> 
> Testing
> -------
> 
> Standard unit tests still pass.
> 
> 
> Thanks,
> 
> John
> 
>