Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.

[David Faure]

> On Jan. 24, 2015, 5:12 p.m., Matthew Dawson wrote:
> > autotests/kconfig_in_global_object.cpp, line 56
> > <https://git.reviewboard.kde.org/r/122232/diff/1/?file=344443#file344443line56>
> >
> >     This doesn't actually trigger anything when run without the fix below.  Since a name is provided in initConfig, it never tries to fetch the global name and thus doesn't test the issue.
> >     
> >     I think these two files should be the exact same, except for testing KSharedConfig vs KConfig.

Yes this unittest is not directly related to the change. It's just that I found it in kdelibs4support and brought it over. Any test is good to have, right?


> On Jan. 24, 2015, 5:12 p.m., Matthew Dawson wrote:
> > autotests/ksharedconfig_in_global_object.cpp, line 52
> > <https://git.reviewboard.kde.org/r/122232/diff/1/?file=344444#file344444line52>
> >
> >     Just before this, could you stick a:
> >     qputenv("QT_FATAL_WARNINGS", "1");
> >     
> >     That way if a warning is printed the test will fail.  Otherwise this change might be removed with tests still passing.

Yep, I completely agree. I actually thought of that some time after submitting the patch, and forgot to make the change. Done now, thanks :)


> On Jan. 24, 2015, 5:12 p.m., Matthew Dawson wrote:
> > src/core/kconfig.cpp, line 542
> > <https://git.reviewboard.kde.org/r/122232/diff/1/?file=344445#file344445line542>
> >
> >     Reading the Qt documentation, I think its possible that appArgs[0] isn't the application name on Windows, and thus appArgs may stay empty.
> >     
> >     Maybe add another conditional checking its empty in this if statement, and if it still is add some random value?

It's possible that appArgs[0] is not the app name, but I'm pretty sure the QStringList we get from QCoreApplication is never empty.


- David


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


On Jan. 23, 2015, 10:39 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122232/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2015, 10:39 p.m.)
> 
> 
> Review request for KDE Frameworks, Albert Astals Cid and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> kconfig_in_global_object.cpp comes from kdelibs4support
> (after porting to Q_GLOBAL_STATIC)
> 
> ksharedconfig_in_global_object.cpp is new (but works with kdelibs4)
> and reproduces Albert's KgDifficulty testcase.
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace 
>   autotests/kconfig_in_global_object.cpp PRE-CREATION 
>   autotests/ksharedconfig_in_global_object.cpp PRE-CREATION 
>   src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a 
> 
> Diff: https://git.reviewboard.kde.org/r/122232/diff/
> 
> 
> Testing
> -------
> 
> Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone.
> 
> 
> Thanks,
> 
> David Faure
> 
>

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