Review Request 117922: Preliminary implementation of adjust signature helper

[Kevin Funk]

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


Code is looks artificially complex at times, which makes it hard to reason about correctness in all the corner cases that crop up. Please try to keep things simple :)


codegen/clangsignatureassistant.h
<https://git.reviewboard.kde.org/r/117922/#comment39838>

    Remove extra indentation level here and below



codegen/clangsignatureassistant.h
<https://git.reviewboard.kde.org/r/117922/#comment39844>

    m_foreignTarget looks unnecessary. (!m_targetUnit.isEmpty() && m_foreignTarget) is always true?
    
    Please try to avoid too many states inside your class.



codegen/clangsignatureassistant.h
<https://git.reviewboard.kde.org/r/117922/#comment39851>

    Not initialized



codegen/clangsignatureassistant.h
<https://git.reviewboard.kde.org/r/117922/#comment39845>

    Never used.



codegen/clangsignatureassistant.cpp
<https://git.reviewboard.kde.org/r/117922/#comment39843>

    Again, 'found' is unnecessary, defn/decl already encodes whether something was found or not.



codegen/clangsignatureassistant.cpp
<https://git.reviewboard.kde.org/r/117922/#comment39842>

    Is that union really needed?



codegen/clangsignatureassistant.cpp
<https://git.reviewboard.kde.org/r/117922/#comment39850>

    Make this a function taking a CXFile, fix the callers.



codegen/clangsignatureassistant.cpp
<https://git.reviewboard.kde.org/r/117922/#comment39849>

    Why is that function needed? Isn't clang_getCursorExtend returning the correct range?
    
    Sorry if I'm missing something.



codegen/clangsignatureassistant.cpp
<https://git.reviewboard.kde.org/r/117922/#comment39846>

    Looks like this block can be factored out into a free function return a KUrl. 'foundCandidate' is redundant, too.
    
    Reduces complexity



codegen/clangsignatureassistant.cpp
<https://git.reviewboard.kde.org/r/117922/#comment39847>

    Merge the two lines;



codegen/clangsignatureassistant.cpp
<https://git.reviewboard.kde.org/r/117922/#comment39848>

    Redundant?



codegen/codeassistant.h
<https://git.reviewboard.kde.org/r/117922/#comment39839>

    Remove extra indentation level here and below



util/clangutils.h
<https://git.reviewboard.kde.org/r/117922/#comment39840>

    Do we really need to have 3 overloads for this?
    
    "bool getCXCursor(int line, int column, CXTranslationUnit unit, CXFile file, CXCursor& cursor);" should be enough.
    
    The callers can just use clang_getFile themselves. That also means 'getFunctionCursor' doesn't need a 'unit' param anymore, can be a normal function, etc. pp.



util/clangutils.cpp
<https://git.reviewboard.kde.org/r/117922/#comment39841>

    -> 'isScopeKind'


- Kevin Funk


On May 2, 2014, 7:56 p.m., David Stevens wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117922/
> -----------------------------------------------------------
> 
> (Updated May 2, 2014, 7:56 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> This is a preliminary implementation of the adjust signature helper. It works fairly well under normal usage, but it runs into problems if the user starts trying to use it at a high frequency. There are some rather fundamental issues both due to the different information organization (files vs translation units) and due to the discrepencies between what KDevelop has in the text editor and what clang has in the translation units. Hopefully I can get some feedback on the implementation.
> 
> 
> Diffs
> -----
> 
>   clangsupport.h 1316f88 
>   clangsupport.cpp 5e3f464 
>   codecompletion/completionhelper.cpp dfaf6b3 
>   codegen/CMakeLists.txt 0b913bd 
>   codegen/clangsignatureassistant.h PRE-CREATION 
>   codegen/clangsignatureassistant.cpp PRE-CREATION 
>   codegen/codeassistant.h PRE-CREATION 
>   codegen/codeassistant.cpp PRE-CREATION 
>   duchain/clangtypes.h c7293a9 
>   duchain/clangtypes.cpp 11760c8 
>   tests/CMakeLists.txt c2fd36d 
>   util/clangutils.h 3516821 
>   util/clangutils.cpp a74017c 
> 
> Diff: https://git.reviewboard.kde.org/r/117922/diff/
> 
> 
> Testing
> -------
> 
> Manual. Any unit tests would be deeply tied into the GUI, so I'm not sure how to go about writing those. The old c++ plugin unfortunately doesn't have any tests to work off of.
> 
> 
> Thanks,
> 
> David Stevens
> 
>

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