Review Request 116842: Added virtual override completion to kdev-clang

[Milian Wolff]

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


Awesome work David, this is looking really good already! I have mostly small nitpicks throughout the review. The big "blocker" currently is the lack of unit tests. Could you look into that please?


codecompletion/context.h
<https://git.reviewboard.kde.org/r/116842/#comment37418>

    here and below: please follow the coding style, i.e. put the opening braces of structs/classes and function bodies on the following line



codecompletion/context.h
<https://git.reviewboard.kde.org/r/116842/#comment37419>

    FunctionParts should not be shared, just make this a QVector<FunctionParts> or similar.
    
    Note that QString is internally shared already so you don't loose anything. And I don't think you need/want pointer semantics in the list here, do you?
    
    also, add a Q_DECLARE_TYPEINFO(FunctionParts, Q_MOVABLE_TYPE)
    
    and furthermore, I prefer the new c++11 syntax for typedefs:
    
    using FunctionPartsList = QVector<FunctionParts>;



codecompletion/context.h
<https://git.reviewboard.kde.org/r/116842/#comment37420>

    unrelated change - undo it if you know how, otherwise leave it in and someone else will undo it prior to commit



codecompletion/context.h
<https://git.reviewboard.kde.org/r/116842/#comment37421>

    use the FunctionPartsList typdef here



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37422>

    since this is only used once, I'd remove the macro here



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37423>

    brace (see above)



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37424>

    use i18n("Override %1", fp.returnType")



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37425>

    brace



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37427>

    here and below: put * and & next to the typename



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37426>

    brace



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37435>

    please put these declarations on separate lines



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37431>

    here and below: rtrim please



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37429>

    same as for the map (see below)



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37428>

    why use ptr-semantics at all for the map here? the ptr-semantics for info should be enough, no?



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37430>

    here and below: please don't use c-style casts



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37433>

    + ' ' +



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37434>

    '(' + ... + ')';



codecompletion/context.cpp
<https://git.reviewboard.kde.org/r/116842/#comment37432>

    remove, QList properly initializes by itself


- Milian Wolff


On March 16, 2014, 7:32 p.m., David Stevens wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116842/
> -----------------------------------------------------------
> 
> (Updated March 16, 2014, 7:32 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> Added virtual override completion to kdev-clang. Regular functions, template functions, and pure functions work. Signals and slots don't work, and const-qualified functions lose their qualifier. I'm not sure if the UI is completely correct.
> 
> 
> Diffs
> -----
> 
>   codecompletion/context.h aa6fcfb 
>   codecompletion/context.cpp f6e85d6 
> 
> Diff: https://git.reviewboard.kde.org/r/116842/diff/
> 
> 
> Testing
> -------
> 
> Manual testing.
> 
> 
> Thanks,
> 
> David Stevens
> 
>

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