Review Request: Add support for subgroups to kconf_update
Oswald Buddenhagen
ossi at kde.org
Sat Apr 17 10:31:39 BST 2010
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3623/#review5081
-----------------------------------------------------------
having never used kconf_update myself and not knowing the code, this is mostly only a formal review as before. therefore you won't get a "ship it" from me - commit at your own peril. :)
trunk/KDE/kdelibs/kconf_update/kconf_update.cpp
<http://reviewboard.kde.org/r/3623/#comment4561>
copyMove*(), etc. are confusing. maybe copyOrMove*()
trunk/KDE/kdelibs/kconf_update/kconf_update.cpp
<http://reviewboard.kde.org/r/3623/#comment4563>
of course, the way this splitting is done prevents group names containing commas (except if somebody had the idea to hex-escape it). i'm not too worried by that, though ...
trunk/KDE/kdelibs/kconf_update/kconf_update.cpp
<http://reviewboard.kde.org/r/3623/#comment4560>
here you could use the method factored out above
trunk/KDE/kdelibs/kconf_update/kconfigutils.cpp
<http://reviewboard.kde.org/r/3623/#comment4562>
not that it would matter here, but you could save an allocation by rewriting this as
str.chop(1);
str.remove(0, 1);
- Oswald
On 2010-04-16 12:34:42, Aurélien Gâteau wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3623/
> -----------------------------------------------------------
>
> (Updated 2010-04-16 12:34:42)
>
>
> Review request for kdelibs.
>
>
> Summary
> -------
>
> This huge patch adds the ability to manipulate subgroups to kconf_update. It is now possible to specify a group either as "group" or as "[group][subgroup]".
>
>
> Diffs
> -----
>
> trunk/KDE/kdelibs/kconf_update/CMakeLists.txt 1115417
> trunk/KDE/kdelibs/kconf_update/README.kconf_update 1115417
> trunk/KDE/kdelibs/kconf_update/kconf_update.cpp 1115417
> trunk/KDE/kdelibs/kconf_update/kconfigutils.h PRE-CREATION
> trunk/KDE/kdelibs/kconf_update/kconfigutils.cpp PRE-CREATION
> trunk/KDE/kdelibs/kconf_update/tests/CMakeLists.txt 1115417
> trunk/KDE/kdelibs/kconf_update/tests/test_kconf_update.cpp 1115417
> trunk/KDE/kdelibs/kconf_update/tests/test_kconfigutils.h PRE-CREATION
> trunk/KDE/kdelibs/kconf_update/tests/test_kconfigutils.cpp PRE-CREATION
>
> Diff: http://reviewboard.kde.org/r/3623/diff
>
>
> Testing
> -------
>
> Unit-tests for kconf_update I wrote before adding support for subgroups still pass. I also extended the existing tests and added new ones to test the subgroup support.
>
>
> Thanks,
>
> Aurélien
>
>
More information about the kde-core-devel
mailing list