Review request: control flow graph plugin

[Andreas Pakulat]

On 09.11.09 14:55:26, Esben Mose Hansen wrote:
> On Saturday 17 October 2009 20:02:06 Andreas Pakulat wrote:
> > - line 54/55 in controlflowgraphview.cpp look like they should be a
> >   normal function call, not s signal/slot connection? Also if you want a
> >   read-write part you should use the create<KParts::ReadWritePart>
> 
> This also took some digging. Apparently, the function call is not possible 
> because the function is defined in the specific KPart, which we I suppose we 
> can't just include and cast to. Instead, I have called the method via. 
> QMetaObject::invokeMethod(...)

We must not rely on internal functions of a KPart, so kgraphviewer needs to
expose this function in a public interface to which we can cast the KPart
instance.
 
> > - deregistering the toolview should be done in the unload method, as
> >   plugin instances are never deleted (until end of the program)
> 
> Ok, I have arranged that the plugin emits unloaded(), which is connected to  
> ControlFlowViewGraph::slotUnloaded(), which in turn call unregisterToolView(). 
> I hope this is correct --- do we have a tutorial or something about how this 
> is supposed to work?

Usually the plugin instance directly calls unregisterToolView() as it knows
the factory. That'll destroy the view and all connected qobject's. Just
look at other toolviews, like the classbrowser.
 
> > - the toolbutton signal/slot connection in controlflowgraphview would
> >   benefit from a QSignalMapper to hide the (IMHO) ugly use of
> >   sender()+object names
> 
> I don't see how this is possible given that the signals we would map have a 
> parameter (bool checked). (Insert rant about Qt primitive signal-slot 
> connections compared to the competition). 

Maybe I didn't look close enough, I thought the parameter is not used (or
can be fetched in another way)

> > - The license headers mention both LGPL and GPL (first paragraph says
> >   LGPL, second says license text of GNU GPL). Also there's no mentioning
> >   of LGPLv2+ etc. (i.e. is it v2, v3, v2+v3 or v2 and any later)
> > 
> 
> Can I fix this at all? I really don't care which license it is as long as they 
> match L?GPLv[2-3], but I can't just change the copyright notice of someone 
> else.

Right, you can't fix that only Sandro can.

> The code is unstable, though, with some memory corruption. Stil working on 
> this. From valgrind, I suspect that this is the problematic code:
> 
> QString DUChainControlFlow::globalNamespaceOrFolderNames(Declaration 
> *declaration)
> {
>     IBuildSystemManager *buildSystemManager;
>     if (m_useFolderName && m_currentProject && (buildSystemManager = 
> m_currentProject->buildSystemManager()))
>     {
>         if (KDevelop::ProjectBaseItem *project_item = m_currentProject-
> >projectItem())
>         {
>             KUrl::List list = buildSystemManager->includeDirectories( 
> project_item );
>             int minLength = std::numeric_limits<int>::max();

Where is m_currentProject coming from? I guess this is somehow calculated
based on the currently open file? If m_currentProject is always a proper
project reference (one needs to monitor the signals from the project
controller!) then the code shouldn't crash.
 
Andreas

-- 
Excellent day for putting Slinkies on an escalator.