Review request: control flow graph plugin

[Esben Mose Hansen]

On Saturday 17 October 2009 20:02:06 Andreas Pakulat wrote:
> Sooooo first round (no in-depth stuff, but I've looked across the files
> a bit):
> 
> - indentation is mixing space and tabs, we usually use space-only, would
>   be nice if you could change that either way so its consistent

Done.

> 
> - DUChainControlFlow::selectionIs seems to be a rather bad function
>   name

Name seems to be inherited from KGraphViewer. After some digging I have 
determined that this is the action that is taken when an element in the graph 
is selected, so I have renamed it as such.

> 
> - 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(...)

> 
> - 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?

> 
> - 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). 

Of course, I could easily split the 2 functions out in a total of 6 functions, 
without any need of a QSignalMapper. In fact, that is what I'll do.


> 
> - 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.

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();

I have attached valgrind's report (pasting makes them unreadable). Look e.g. 
at the very last problem reported (there are 4 or 5 similar problems reported) 
If anyone can spot something obvious wrong, it'd be wonderful, otherwise I'll 
keep digging.

-- 
Kind regards, Esben
-------------- next part --------------
A non-text attachment was scrubbed...
Name: valgrind.kdevelop.log.zip
Type: application/zip
Size: 8804 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20091109/c2f2eb4d/attachment.zip>