Refactor and fix DU-Chain creation
ClosedPublic

Authored by ematirov on Jun 27 2017, 7:51 PM.

Details

Summary

As result, function body context is added as internal context and args / return vars contexts are added as imported to internal.

Test Plan

Tests pass. Wiped cache and started KDevelop - standard libs are parsed without faults.

Diff Detail

Repository
R59 KDevelop Go
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ematirov created this revision.Jun 27 2017, 7:51 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJun 27 2017, 7:51 PM
brauch accepted this revision.Jun 27 2017, 8:57 PM

Seems sensible to me from a first look, certainly more than it was before ;)

codecompletion/items/functionitem.cpp
50

use DUChainUtils::getArgumentContext

duchain/tests/testduchain.cpp
720

you can use auto

This revision is now accepted and ready to land.Jun 27 2017, 8:57 PM
mwolff accepted this revision.Jun 28 2017, 8:15 AM
mwolff added a subscriber: mwolff.

overall certainly an improvement, some suggestions from my side

duchain/builders/declarationbuilder.cpp
587

a context for the return parameter? is that really required? just asking out of curiosity, afaik the other language plugins don't do that (or?)

606

are you sure that this always creates a context? i.e. even if the user typed somewhat broken code into the editor? I suggest you better rewrite this like the below, to be on the safe side:

if (block) {
    visitBlock(block);
    DUChainWriteLocker lock;
    bodyContext = lastContext();
    if (bodyContext) {
        bodyContext->setType(DUContext::Other);
    }
}

better yet, could you set this type directly wherever you are creating the context itself? from a cursory glance, I don't even see where this is done

duchain/builders/declarationbuilder.h
71

nitpicks here and below: please try to follow our coding style and add spaces around operators (such as the = here

also, you can use = {} to reduce some typing repetition here for the comment

105

dito, see above

ematirov updated this revision to Diff 15942.Jun 28 2017, 9:39 AM

Use review suggestion.

ematirov marked 5 inline comments as done.Jun 28 2017, 9:47 AM

Thank you for your review!

duchain/builders/declarationbuilder.cpp
587

I think that it's handful because in Go language "named result parameters " exists: https://golang.org/doc/effective_go.html#named-results.
We need to provide declarations for them because they are declared inside function context and have zero values by default. See this snippet for example of usage. Aside from that, having them in separate context is useful for showing them as part of function signature. Since there could be more than one return value in Go it provides a handful documentation:

606

You were right, there is no need for calling setType since visitBlock already opens context with correct type.

mwolff accepted this revision.Jun 29 2017, 9:51 AM

thanks for the explanation - I have zero clue about go, but looking at that snippet I can understand why you want another context there.

Cool stuff!

This revision was automatically updated to reflect the committed changes.
ematirov marked an inline comment as done.