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/#review57235
-----------------------------------------------------------


Can't test your patch, because it asserts early.

#5  0x00007ffff5e73cce in qt_assert (assertion=0x7ffff3d38ab8 "DUChain::lock()->currentThreadHasReadLock() || DUChain::lock()->currentThreadHasWriteLock()", file=0x7ffff3d38330 "/home/krf/devel/src/kdevp
latform/language/duchain/duchain.cpp", line=1312) at /home/krf/devel/src/qt/src/corelib/global/qglobal.cpp:2054
#6  0x00007ffff3c13a95 in KDevelop::DUChain::chainForDocument (this=this at entry=0x244cd90, document=..., proxyContext=proxyContext at entry=false) at /home/krf/devel/src/kdevplatform/language/duchain/duchain
.cpp:1312
#7  0x00007ffff3c13e57 in KDevelop::DUChain::chainForDocument (this=0x244cd90, document=..., proxyContext=<optimized out>) at /home/krf/devel/src/kdevplatform/language/duchain/duchain.cpp:1274
#8  0x00007ffff3c7e409 in KDevelop::DUChainUtils::standardContextForUrl (url=..., preferProxyContext=preferProxyContext at entry=false) at /home/krf/devel/src/kdevplatform/language/duchain/duchainutils.cpp:
277
#9  0x00007fff4be267ae in (anonymous namespace)::getSession (url=...) at /home/krf/devel/src/kdev-clang/codegen/clangsignatureassistant.cpp:56
#10 0x00007fff4be26e45 in ClangSignatureAssistant::textChanged (this=this at entry=0x41fa190, change=[(1, 7) -> (3, 4)]) at /home/krf/devel/src/kdev-clang/codegen/clangsignatureassistant.cpp:297
#11 0x00007fff4be23c4f in CodeAssistant::eventuallyStartAssistant (this=0x35c94d0) at /home/krf/devel/src/kdev-clang/codegen/codeassistant.cpp:214


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

    Nitpick:
    - Newline before {.
    - Indent Q_OBJECT
    



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

    I suggest holding the DUChainReaderLocker here.
    
    Just tested your patch and got an assert while accessing this function from within the 'textChanged' method.



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

    API change: return the CXCursor:
    
    This will make the code simpler, too:
    
    CXCursor cursor = ClangUtils::getCXCursor(sc.line, sc.column, unit, file));
    if (clang_getCursorKind(cursor) == CXCursor_ParmDecl) {
        cursor = clang_getCursorSemanticParent(cursor);
    }
    if (!CursorKindTraits::isFunction(clang_getCursorKind(cursor)))
        return clang_getNullCursor();
    return cursor;



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

    Do we need the lock here?



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

    Just use 'clang_getFile(..., m_otherLoc.document.byteArray().constData())'?



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

    Return the CXCursor


- Kevin Funk


On May 3, 2014, 1:31 a.m., David Stevens wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117922/
> -----------------------------------------------------------
> 
> (Updated May 3, 2014, 1:31 a.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/20140504/36e00be7/attachment-0001.html>