Review Request: Checking infrastructure for the KDevPlatform

Mon Nov 7 23:23:12 UTC 2011

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

generally fine with me, just a few things that need to be improved first please.

language/backgroundparser/parsejob.h
<http://git.reviewboard.kde.org/r/103071/#comment6873>

    data modification inside the parsed file?

    maybe: structure containing information about data modifications inside the parsed file?

language/checks/controlflowgraph.h
<http://git.reviewboard.kde.org/r/103071/#comment6874>

    open this file in katepart, press F7, enter "rtrim", hit enter.

    this removes the trailing spaces.

language/checks/controlflowgraph.h
<http://git.reviewboard.kde.org/r/103071/#comment6876>

    all add* methods should say what happens to ownership. I assume the graph takes ownership?

language/checks/controlflowgraph.h
<http://git.reviewboard.kde.org/r/103071/#comment6875>

    given *by* ?

    or better:

    adds an entry @p n associated with the declaration @p decl to the graph

    @see declarations()

language/checks/controlflowgraph.h
<http://git.reviewboard.kde.org/r/103071/#comment6877>

    I'd prefer nodeForDeclaration instead of Per

language/checks/controlflowgraph.h
<http://git.reviewboard.kde.org/r/103071/#comment6878>

    I'd rename this to nodes() or rootNodes()

    the class is a Graph, and as such code like this won't be uncommon:

    ControlFlowGraph* graph;
    graph->graphNodes(); // meh :-/

language/checks/controlflowgraph.cpp
<http://git.reviewboard.kde.org/r/103071/#comment6879>

    this leaks the d pointer

    either make it a QScopedPointer or delete it here

language/checks/controlflownode.h
<http://git.reviewboard.kde.org/r/103071/#comment6880>

    again, please rtrim

language/checks/controlflownode.h
<http://git.reviewboard.kde.org/r/103071/#comment6881>

    public api, please don't inline code to make it easier to maintain. same goes for the methods below

    I doubt this is a performance issue here.

language/checks/controlflownode.h
<http://git.reviewboard.kde.org/r/103071/#comment6883>

    public api, should get a dptr for maintainability, or?

language/checks/controlflownode.cpp
<http://git.reviewboard.kde.org/r/103071/#comment6884>

    why is m_nodeRange not initialized with invalid?

language/checks/controlflownode.cpp
<http://git.reviewboard.kde.org/r/103071/#comment6885>

    rtrim :)

language/checks/controlflownode.cpp
<http://git.reviewboard.kde.org/r/103071/#comment6886>

    please put spaces around operators

language/checks/dataaccessrepository.h
<http://git.reviewboard.kde.org/r/103071/#comment6887>

    rtrim :)

language/checks/dataaccessrepository.h
<http://git.reviewboard.kde.org/r/103071/#comment6891>

    personally I'd prefer if each class has it's own .cpp + .h filepair.

language/checks/dataaccessrepository.h
<http://git.reviewboard.kde.org/r/103071/#comment6888>

    please put the None on its own line, I missed it the first time

language/checks/dataaccessrepository.h
<http://git.reviewboard.kde.org/r/103071/#comment6889>

    again: please no inline code for public api

language/checks/dataaccessrepository.h
<http://git.reviewboard.kde.org/r/103071/#comment6890>

    again: please d-ptr for maintainability - or?

language/checks/dataaccessrepository.h
<http://git.reviewboard.kde.org/r/103071/#comment6892>

    no inline code please

    also - no ctor? to make it easier to maintain I'd add that just to make sure.

language/checks/dataaccessrepository.h
<http://git.reviewboard.kde.org/r/103071/#comment6893>

    again: dptr

language/checks/dataaccessrepository.cpp
<http://git.reviewboard.kde.org/r/103071/#comment6894>

    spaces around operators please

language/checks/dataaccessrepository.cpp
<http://git.reviewboard.kde.org/r/103071/#comment6895>

    spaces please

language/checks/dataaccessrepository.cpp
<http://git.reviewboard.kde.org/r/103071/#comment6896>

    rtrim please

- Milian Wolff

On Nov. 7, 2011, 10:48 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103071/
> -----------------------------------------------------------
> 
> (Updated Nov. 7, 2011, 10:48 p.m.)
> 
> 
> Review request for KDevelop, Milian Wolff and David Nolden.
> 
> 
> Description
> -------
> 
> As some of you will know, my master thesis was built on top of kdevelop with the idea to add some static analysis capabilities to KDevelop/KDevPlatform by adding some new tools and stuff. A document describing what I did can be found here: http://proli.net/meu/pfc/memoria.pdf
> 
> In this patch there's the changes I made in the KDevPlatform, mostly to add the DataAccessRepository and the ControlFlowGraph (Chapter 2) data types and the ILanguageCheck and ILanguageCheck provider (Chapter 3).
> 
> If there's any question just ask me :). I'll submit another patch for KDevelop, implementing these features shortly.
> 
> 
> Diffs
> -----
> 
>   language/CMakeLists.txt eb85b2c 
>   language/backgroundparser/parsejob.h 135319c 
>   language/checks/controlflowgraph.h PRE-CREATION 
>   language/checks/controlflowgraph.cpp PRE-CREATION 
>   language/checks/controlflownode.h PRE-CREATION 
>   language/checks/controlflownode.cpp PRE-CREATION 
>   language/checks/dataaccessrepository.h PRE-CREATION 
>   language/checks/dataaccessrepository.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103071/diff/diff
> 
> 
> Testing
> -------
> 
> Well, the testing is what I've built on top. It's coming, I'm just fixing some issues after merging from master. It wasn't as bad as I expected though ;).
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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