Review Request: Include resolving using 'make' optional

[Alexandre Courbot]

> On May 14, 2012, 1:15 a.m., Alexandre Courbot wrote:
> > I noticed that this patch has been reverted a while ago:
> > 
> > commit 17fa62314a30071296533e8e1cd53d9fca730f18
> > Author: Milian Wolff <mail at milianw.de>
> > Date:   Tue Mar 20 17:02:20 2012 +0100
> > 
> >     Revert "make include-path resolution using make optional on a per-project basis"
> >     
> >     include-path auto completion creates a includepath computer in the background
> >     thread which would break this patch (and assert, actually).
> >     
> >     shows that we desparately need a unit test for this. I'll revive the patch
> >     once I have time to fix it properly.
> >     
> >     This reverts commit ff1744bcb8b125f6bcb0227925973a59d176de39.
> > 
> > Milian, could you elaborate on what needs to be fixed for this patch to make it again? I'd like to give it a try. Thanks.
> 
> Milian Wolff wrote:
>     Oh sorry! I really meant to work on it...
>     
>     The issue is basically that the project model was accessed from a background thread, through findIncludePaths() in cpputils.cpp which is e.g. eventually used via CodeCompletionContext::doIncludeCompletion (well, just revert the revert and try to do include-path auto completion and get a backtrace, you'll see where it asserts). One potential fix for that would be to wrap the code in the includepathcomputer with the foregroundlock, you could try that out... Not sure there is a better alternative I'm afraid :(

Sorry, I still fail to understand. The project controller and project model are only accessed read-only by the background thread and are only used to read a configuration option - how can this mess with include-path auto completion and cause a crash?

I can see the following assert in IncludePathComputer:

Q_ASSERT(QThread::currentThread() == QCoreApplication::instance()->thread());

I understand the issue of having that run in a background thread, but unless I missed something this is not what the current patch is doing - or is it?


- Alexandre


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


On Oct. 16, 2011, 8:31 a.m., Alexandre Courbot wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102883/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2011, 8:31 a.m.)
> 
> 
> Review request for KDevelop and Milian Wolff.
> 
> 
> Description
> -------
> 
> Include resolving using 'make' optional
> 
> Make can be invoked by the background parser if some include paths are
> unresolved. However some projects do not use make for building and this
> potentially induces an overhead for large projects. This patch adds a
> project-wide option to disable this behavior.
> 
> 
> Diffs
> -----
> 
>   languages/cpp/includepathresolver.cpp d0b1cc1ede30409f77123573b269ea866a71ff96 
>   projectbuilders/makebuilder/makebuilderconfig.kcfg 8b521f12a92ce1bddef51c6a4a4126d8c9d1893c 
>   projectbuilders/makebuilder/makeconfig.ui 6c047c1bda5ec8ffc9af98546a33d083e065185b 
> 
> Diff: http://git.reviewboard.kde.org/r/102883/diff/
> 
> 
> Testing
> -------
> 
> Checked that the setting is correctly set and taken into account by the IncludePathResolver.
> 
> 
> Thanks,
> 
> Alexandre Courbot
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120516/09951f48/attachment.html>