As result, function body context is added as internal context and args / return vars contexts are added as imported to internal.
Details
- Reviewers
brauch apol mwolff - Commits
- R59:a61ed96f2688: Refactor and fix DU-Chain creation
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.
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 |
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. | |
606 | You were right, there is no need for calling setType since visitBlock already opens context with correct type. |
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!