Review Request 116845: Add KCoreConfigSkeleton::read() which doesn't call reparseConfiguration.

Mon Mar 17 23:15:18 UTC 2014

> On March 17, 2014, 3:10 a.m., Matthew Dawson wrote:
> > The unit test situation seems far from ideal, but since it fits with the current setup it can be left for now.  Changing them can be left for another patch :)
> > 
> > Otherwise, I imagined readConfig would become a regular method, and read is the new virtual method to implement to override the KConfig parsing.  I've opened some issues about that below.
> > 
> > Also, I'd like to get the usrReadConfig function renamed in this patch, as we were discussing in RR #116461, unless we decide to leave the name alone.

I think we shouldn't rename usrReadConfig (reasoning in 116461).

> On March 17, 2014, 3:10 a.m., Matthew Dawson wrote:
> > src/core/kcoreconfigskeleton.h, line 1055
> > <https://git.reviewboard.kde.org/r/116845/diff/1/?file=254551#file254551line1055>
> >
> >     I can't find any instances (lxr.kde.org returns way too many results to thoroughly check) where readConfig() was overridden and I don't know why you would want to override it, but since some users may have anyways I'd prefer if read() stayed a virtual for compatibility, since it replaces readConfig.

It doesn't really replace readConfig(). We offer both: readConfig() (soon to be renamed load()) which reloads from disk, and read() which only reads from memory.

I grepped my kde source code along the lines of grep Skeleton `rgrep -l 'void.*readConfig'` and I can't find any reimplementations of readConfig() either. I think it was back in the days of making things virtual "just in case", like in Qt3 (which then got massively cleaned up for Qt4). We might want to do the same. Meanwhile this confirms that read() is fine as I wrote it....

If anyone needs to change behavior there's usrReadConfig() anyway - or the virtual readConfig() in each item. So I see no point in readConfig() or read() being virtual.

> On March 17, 2014, 3:10 a.m., Matthew Dawson wrote:
> > src/core/kcoreconfigskeleton.h, line 1052
> > <https://git.reviewboard.kde.org/r/116845/diff/1/?file=254551#file254551line1052>
> >
> >     Regarding the virtual methods, once they change this needs to be updated.

> On March 17, 2014, 3:10 a.m., Matthew Dawson wrote:
> > src/core/kcoreconfigskeleton.h, line 1041
> > <https://git.reviewboard.kde.org/r/116845/diff/1/?file=254551#file254551line1041>
> >
> >     Can you remove the virtual from readConfig, since there isn't a reason to override readConfig, and may in fact break if other code uses read() instead.

I agree (see 2 issues further down for my grepping). It's a separate commit though, this one doesn't touch readConfig(). But yeah I'll do that.

> On March 17, 2014, 3:10 a.m., Matthew Dawson wrote:
> > src/core/kcoreconfigskeleton.cpp, line 989
> > <https://git.reviewboard.kde.org/r/116845/diff/1/?file=254552#file254552line989>
> >
> >     I'd prefer to keep the same behaviour we implemented before.  This allows for applications to make use of the optimization if they haven't been ported yet.
> >     
> >     It also reduces developer thought in the simple case, as they can just always call readConfig/loadConfig, and be ensured they get the latest values from disk, while avoiding an extra read off disk.  That leaves the read() function there as an optimization for the more advanced cases.

Well, the apps don't need any porting if they ask kconfig_compiler to generate a singleton for them (which calls read instead of readConfig).
If they don't use a singleton, then, well, that's what I test in "test1", and I just debugged it, and we both missed something:

KCoreConfigSkeleton::addItem calls item->readConfig. So the reading from KConfig happens automatically, without the need for apps to call readConfig at all. The singleton-generating code is just stupid, it all happens automatically without a call to readConfig (except of course if apps wanted to reimplement that...)

So DelayedParsing breaks the Test1 case (constructor, no explicit call to readConfig/read). Yay for unittests.

- David

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116845/#review53151
-----------------------------------------------------------

On March 16, 2014, 11:08 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116845/
> -----------------------------------------------------------
> 
> (Updated March 16, 2014, 11:08 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> Add KCoreConfigSkeleton::read() which doesn't call reparseConfiguration.
> 
> Call it from generated singletons, since the constructor creates
> a KConfig from a filename, which already loads from disk.
> This removes the need for using DelayedParsing.
> 
> 
> Diffs
> -----
> 
>   autotests/kconfig_compiler/test4main.cpp 8f1c1ec8d41ea123893a59526c52cdbd0b5ee075 
>   autotests/kconfig_compiler/test5.cpp.ref 088cc78f4dc96a719628ece342e149553c1d22aa 
>   autotests/kconfig_compiler/test8b.cpp.ref dcd61693ff86f02eaeb93b4c4da9e6ed20463469 
>   autotests/kconfig_compiler/test_dpointer.cpp.ref e50bf8aa29a2d009c4156dabf429c3ffb74eb7ba 
>   autotests/kconfig_compiler/test_signal.cpp.ref 35b5cba2736761753d1ba32baa6f9fc2e7808ba1 
>   autotests/kconfigskeletontest.cpp 31278e767655f7e3d25a4ed9984345e8c4e67d82 
>   src/core/kcoreconfigskeleton.h a2b828a4ef3f881b551049901d4bc26bb23a014b 
>   src/core/kcoreconfigskeleton.cpp 0c1a96faaa9c511d349a26ad8af4b7b2c9bf313e 
>   src/kconfig_compiler/kconfig_compiler.cpp 7d84cfbc28b1648ca999116726455183d88a7f0a 
>   autotests/kconfig_compiler/test4.cpp.ref 66d0357f114b0aca6148a2091880dd2f62fbf624 
>   autotests/kconfig_compiler/test1main.cpp d7dc038d91d2f8babcd281960100d1c6fa264d7c 
>   autotests/kconfig_compiler/test10.cpp.ref 21aea9d87af5fce01e64032257283e0883af2d81 
> 
> Diff: https://git.reviewboard.kde.org/r/116845/diff/
> 
> 
> Testing
> -------
> 
> Added two unittests (which is how I discovered I had to remove DelayedParsing, so the tests were useful).
> 
> The "bool ok + qWarning" thing is a bit clumsy, maybe we want to port these main() to qtestlib too, even though they are themselves executed by a unittest :-)
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140317/95a2f947/attachment-0001.html>