Refactoring CMake support

Fri Sep 6 01:57:07 UTC 2013

On Wed, Aug 28, 2013 at 4:16 AM, Aleix Pol <aleixpol at kde.org> wrote:

> On Tue, Aug 27, 2013 at 10:48 AM, Milian Wolff <mail at milianw.de> wrote:
>
>> On Tuesday 27 August 2013 02:10:36 Aleix Pol wrote:
>> > I haven't had problems with it so far, I'll merge it later this week.
>> I'm
>> > not doing it yet because I just merged the noAreaTabs branch and I'd
>> prefer
>> > to break systems one at a time.
>> >
>> > In any case, please test :)
>>
>> I've successfully loaded my big kdev* session with 10 cmake projects in
>> it. So
>> it works - cool!
>>
>> Switching branches in kdevplatform from master to 1.4 triggers a crash
>> though:
>> Can you reproduce this?
>>
>> Thread 1 (Thread 0x7fc905a927c0 (LWP 17518)):
>> [KCrash Handler]
>> #5  0x00007fc86938b978 in CMakeManager::isReloading (this=0x30131b0,
>> p=0x0) at
>>
>> /ssd/milian/projects/kde4/kdevelop/projectmanagers/cmake/cmakemanager.cpp:649
>> #6  0x00007fc86938be10 in CMakeManager::realDirectoryChanged
>> (this=0x30131b0,
>> dir=...) at
>>
>> /ssd/milian/projects/kde4/kdevelop/projectmanagers/cmake/cmakemanager.cpp:685
>> #7  0x00007fc86938bcf7 in CMakeManager::filesystemBuffererTimeout
>> (this=0x30131b0) at
>>
>> /ssd/milian/projects/kde4/kdevelop/projectmanagers/cmake/cmakemanager.cpp:676
>> #8  0x00007fc86937b83c in CMakeManager::qt_static_metacall (_o=0x30131b0,
>> _c=QMetaObject::InvokeMetaMethod, _id=6, _a=0x7fffad95a410) at
>>
>> /ssd/milian/projects/.build/kde4/kdevelop/projectmanagers/cmake/moc_cmakemanager.cpp:71
>> #9  0x00007fc916068ba8 in QMetaObject::activate(QObject*, QMetaObject
>> const*,
>> int, void**) () from /usr/lib/libQtCore.so.4
>> #10 0x00007fc91606cd71 in QObject::event(QEvent*) () from
>> /usr/lib/libQtCore.so.4
>> #11 0x00007fc9151db12c in QApplicationPrivate::notify_helper(QObject*,
>> QEvent*) () from /usr/lib/libQtGui.so.4
>> #12 0x00007fc9151e16f0 in QApplication::notify(QObject*, QEvent*) () from
>> /usr/lib/libQtGui.so.4
>> #13 0x00007fc91687472a in KApplication::notify(QObject*, QEvent*) () from
>> /usr/lib/libkdeui.so.5
>> #14 0x00007fc916054efd in QCoreApplication::notifyInternal(QObject*,
>> QEvent*)
>> () from /usr/lib/libQtCore.so.4
>> #15 0x00007fc916084633 in ?? () from /usr/lib/libQtCore.so.4
>> #16 0x00007fc916081989 in ?? () from /usr/lib/libQtCore.so.4
>> #17 0x00007fc90f11dd96 in g_main_context_dispatch () from
>> /usr/lib/libglib-2.0.so.0
>> #18 0x00007fc90f11e0e8 in ?? () from /usr/lib/libglib-2.0.so.0
>> #19 0x00007fc90f11e18c in g_main_context_iteration () from
>> /usr/lib/libglib-2.0.so.0
>> #20 0x00007fc916081b85 in
>> QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>)
>> ()
>> from /usr/lib/libQtCore.so.4
>> #21 0x00007fc9152782e6 in ?? () from /usr/lib/libQtGui.so.4
>> #22 0x00007fc916053b5f in
>> QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from
>> /usr/lib/libQtCore.so.4
>> #23 0x00007fc916053e55 in
>> QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from
>> /usr/lib/libQtCore.so.4
>> #24 0x00007fc916058f8b in QCoreApplication::exec() () from
>> /usr/lib/libQtCore.so.4
>> #25 0x00000000004127db in main (argc=3, argv=0x7fffad95c998) at
>> /ssd/milian/projects/kde4/kdevelop/app/main.cpp:561
>>
>> Furthermore looking at the code I have some things to note:
>>
>> a) Don't use QList in new code.
>> http://marcmutz.wordpress.com/effective-qt/containers/ Instead use
>> QVector and mark custom structs as Q_MOVABLE_TYPE.
>>
>> b) Don't use QMap if you don't need sorting (which you probably don't).
>> Use
>> QHash. http://woboq.com/blog/qmap_qhash_benchmark.html Yes, for small
>> sets
>> QMap may be faster but then operations are really fast anyways on small
>> scale.
>>
>> c) Remove trailing spaces - come on Aleix ;-) Do me that favor pretty
>> please?
>> You can just enable the "remove trailing spaces on edited lines" feature
>> in
>> Kate - it's perfect!
>>
>> d) Some other style nitpicks like spaces around operators sometimes
>> missing
>> and placement of * or & not always following the style we use in other
>> places
>> (i.e. attach it to the type name).
>>
>> But don't get me wrong - the code looks really good all in all! The above
>> is
>> just nitpicks, which you could also ignore and I'll refactor eventually.
>> Take
>> it more as an educative post-review :)
>>
>> Many many thanks for working on this! /me is so happy that "grep -R
>> QMutex"
>> returns zero matches in the cmake folder now :) You are my hero Aleix!
>>
>> > On Tue, Aug 20, 2013 at 2:56 AM, Aleix Pol <aleixpol at kde.org> wrote:
>> > > Hi!
>> > > It's not news that we've had some threading issues with the project
>> model,
>> > > especially from the cmake plugin. This was mostly because the project
>> was
>> > > parsed in a different thread and the results where being added from
>> that
>> > > thread to the ProjectsModel. It worked, but killing a bug made
>> another one
>> > > to appear, so that was no fun.
>> > >
>> > > With this change, I'm making it so the project will be parsed from a
>> > > separate thread and will pass the data to another thread that will do
>> the
>> > > tree creation.
>> > >
>> > > I haven't tested it (at all, despite small tests with test projects)
>> but
>> > > the unit tests pass and I'm feeling quite positive about it. I'll be
>> > > testing it in my system making sure all's in order during the
>> following
>> > > weeks, but for now, if somebody is curious enough, I'm interested in
>> any
>> > > kind of feedback.
>> > >
>> > > Cheers!
>> > > Aleix
>> > >
>> > > PS: I'm applying this patch in my kdevplatform, for further safety ;)
>> > > http://paste.opensuse.org/63054558
>>
>> --
>> Milian Wolff
>> mail at milianw.de
>> http://milianw.de
>>
>
> Fixed a) and b), and a couple of more issues I found, still haven't been
> able to reproduce your ASSERT: "ctx".
>
> Aleix
>

This reloading problem should be fixed now, if no further bugs appear, I'll
merge next monday.

Aleix
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130906/f429e063/attachment.html>