Review Request 117527: Added implement function completion to kdev-clang

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117527/#review56218
-----------------------------------------------------------


Wow, color me amazed. Awesome contribution. I hope you don't get annoyed by my nitpicks (which most of the below comments are). Overall, this looks very promising. Thanks!


codecompletion/completionhelper.h
<https://git.reviewboard.kde.org/r/117527/#comment39274>

    please rename the include guards to match the file name



codecompletion/completionhelper.h
<https://git.reviewboard.kde.org/r/117527/#comment39281>

    could be forward-declared



codecompletion/completionhelper.h
<https://git.reviewboard.kde.org/r/117527/#comment39275>

    Remove the get prefix and make the functions const



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39278>

    these helper classes and functions should be put into an anonymous namespace to give them file-local linkage.



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39276>

    QHash?



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39277>

    QVector?



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39279>

    put brace on a newline.
    
    And personally, I'd rename the function to namespaceEquals - but you don't have to change that.



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39280>

    this does a CXString -> QString conversion for both, before doing the comparison. Maybe rather add an operator== to ClangString which uses strcmp on the string returned by clang_getCString() ?



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39282>

    remove get prefix, and return the type arguments by value, i.e.:
    
    QStringList templateTypeArguments(CXCursor cursor)
    {
      QStringList ret;
      ...
      return ret;
    }
    
    Also, this code could reuse ParamIterator from stringhelpers.h in kdevplatform. 



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39283>

    again -get and put the brace on a new line



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39284>

    again -get and put the brace on a new line and return types by value:
    
    QStringList templateParams(CXCursor cursor)
    {
      QStringList params;
      clang_visitChildren(cursor, &templateParamsHelper, &params);
      return params;
    }



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39285>

    I'd welcome an explicit check on the USR length to prevent this from crashing when length < 2.



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39286>

    join with previous line



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39287>

    space after if (also below please)



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39288>

    for multi-line conditionals, please put the brace on its own line.



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39289>

    && name.isEmpty()
    
    instead of name == ""



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39290>

    spaces after comma here (also below)



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39291>

    why is this a list and then joined with an empty separator? Use a QString directly, no?



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39292>

    ... couldn't find file:" << file;



codecompletion/completionhelper.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39293>

    ... position: " << position << file;



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39294>

    that should be handled automatically when a proper indentation configuration is set for kdevelop (e.g. settings -> source formatter). At least it should work this way :)



tests/codecompletiontest.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39297>

    Personally, I prefer this style:
    
    TestFile file("\n"
                  "int foo(char c, int i);\n"
                  "\n", "cpp");



tests/codecompletiontest.cpp
<https://git.reviewboard.kde.org/r/117527/#comment39296>

    here and below: spaces after commas 


- Milian Wolff


On April 22, 2014, 2:21 a.m., David Stevens wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117527/
> -----------------------------------------------------------
> 
> (Updated April 22, 2014, 2:21 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> Added implement function completion. This involved refactoring some code, so the diff is pretty large. The core of the implemenation occurs in lines 186-286 and lines 312-326 of completionhelper.cpp.
> 
> 
> Diffs
> -----
> 
>   codecompletion/CMakeLists.txt e72f5ba 
>   codecompletion/completionhelper.h PRE-CREATION 
>   codecompletion/completionhelper.cpp PRE-CREATION 
>   codecompletion/context.h b328ae9 
>   codecompletion/context.cpp 9f92251 
>   codecompletion/overridecompletionhelper.h fc34275 
>   codecompletion/overridecompletionhelper.cpp aff69f6 
>   tests/codecompletiontest.h bacab65 
>   tests/codecompletiontest.cpp 2ae64c6 
> 
> Diff: https://git.reviewboard.kde.org/r/117527/diff/
> 
> 
> Testing
> -------
> 
> Manual and unit tests.
> 
> 
> Thanks,
> 
> David Stevens
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140422/195c6a9a/attachment-0001.html>