Port ClangClassHelper to also fill the include_files var
ClosedPublic

Authored by kossebau on Feb 3 2017, 5:23 PM.

Details

Summary

The "included_files" variable should contain the include directive arguments
for the base classes of the created class.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau updated this revision to Diff 10902.Feb 3 2017, 5:23 PM
kossebau retitled this revision from to WIP: Port ClangClassHelper to also fill the include_files var.
kossebau updated this object.
kossebau added reviewers: KDevelop, mwolff.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptFeb 3 2017, 5:23 PM
kossebau planned changes to this revision.Feb 3 2017, 5:23 PM
kossebau added inline comments.Feb 3 2017, 5:25 PM
languages/clang/kdevclangsupport.json
56 ↗(On Diff #10902)

Just in this patch as needed to get the language plugin used by the filetemplate plugin, no longer needed once D4424 is in.

mwolff edited edge metadata.Feb 3 2017, 5:35 PM

lgtm in principle

languages/clang/codegen/clangclasshelper.cpp
274–275

while at it: join line

languages/clang/duchain/unknowndeclarationproblem.cpp
335

fitIt?! what? :P

here and below: remove excess KDevelop:: qualification (see above, namespace is used)

541

needs a unit test, missing so far if I'm not mistaken

util stuff is usually put into duchainutils

547

look it up yourself :) potentially there is a fallback, alternatively do the fallback yourself. afaik the project managers will usually return $something for a folder as well.

560

that is for finding an include from the name of a class (i.e. an identifier) - doesn't seem to be relevant here

567

wrong comment

kossebau updated this revision to Diff 10905.Feb 3 2017, 9:12 PM
kossebau marked 4 inline comments as done.
kossebau edited edge metadata.

Small update which picks up some first comments + adds minor improvements
more, like moving code around + tests, to come in the next days

kossebau planned changes to this revision.Feb 3 2017, 9:12 PM
kossebau updated this revision to Diff 12291.Mar 8 2017, 1:11 PM

only export UnknownDeclarationProblem::findMatchingIncludeFiles,
keep custom logic for not-existing files specific to classhelper

kossebau retitled this revision from WIP: Port ClangClassHelper to also fill the include_files var to Port ClangClassHelper to also fill the include_files var.Mar 8 2017, 1:12 PM
kossebau edited the summary of this revision. (Show Details)

Still no matching unit tests added for now. There are two things that could see tests:

  1. the exposed UnknownDeclarationProblem::findMatchingIncludeFiles(...) method
  2. the included_files variable properly filled with data

Both things will need me to invest some more time, as I yet have to grasp things around DUChain enough to write a proper test for the exposed method.
And classhelper code is without any testing approach so far, from what I found (also nothing seen in oldcpp), so that needs some more thinking how to do this properly.

Given 5.1 is planned to be released this week and me lacking enough time for the above now, I propose to have the current patch in already without dedicated unit tests, as it fixes a regression for cpp->clang when it comes to code generation from file templates (missing included_files variable content). Given my dissatisfaction with the current code generation abilities, you should see me working on tests in the near future at least, as part of other development.

kfunk accepted this revision.Mar 12 2017, 10:27 PM
kfunk added a subscriber: kfunk.

Rest LGTM, patch should go into 5.1.

The new code introduced in this patch will only be executed while creating from file templates, so there can't be potential regression in the Clang navigation / code completion support. But we'll fix generated code, which is good.

languages/clang/codegen/clangclasshelper.cpp
103

Could turn that into a simple function, or even inline as lambda at the call-site

181

auto i -> const auto& fileItem

192

includepaths -> includePaths

201

includefiles -> includeFiles

This revision is now accepted and ready to land.Mar 12 2017, 10:27 PM
This revision was automatically updated to reflect the committed changes.
kossebau marked 4 inline comments as done.