It was incorrectly reporting inner classes as inheriters.
Details
Details
Diff Detail
Diff Detail
- Repository
- R33 KDevPlatform
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Just a question: Is the clang builder correct in importing the base for the case you have in your unit test?
class foo { class inner {}; };Why is inner importing foo - does anyone know? If that wouldn't be the case, then this patch here wouldn't be required. So before we accept this patch, I'd appreciate if someone could dig in a bit deeper and clarify whether it isn't a bug in kdev-clang that triggers the issue, instead of a generic DUChain issue here.
| language/duchain/duchainutils.cpp | ||
|---|---|---|
| 485 | use auto | |
| 490 | please merge the conditionals and add braces. also use longer variable names: FOREACH_FUNCTION(const auto& base, childClass->baseClasses) {
auto structureType = base.baseClass.type<StructureType>());
if (structureType && structureType->declaration(topContext) == base) {
return true;
}
}The surrounding code really should be cleaned up by someone from us eventually... Not your fault/concern of course. | |
Comment Actions
I am not sure about the importing stuff. I thought that it visualizes a dependency between types, like type Foo imports Bar because it cannot work without the Bar.
If that is indeed the case, then importing might be a good idea. You cannot lookup the id of the inner class without the outer class. But the basic definition of the inner class should not implicitly depend on the outer class.
Regardless, the code that marks the outer class as an import of the inner class is in language/clang/duchain/builder.cpp:423 in Visitor::createContext method. According to the code, every method and class imports its semantic parent. It might be a good idea for out-of-line method/class definition, but it is done even if semantic parent == lexical parent.
Comment Actions
In the context of kdev-clang, "should" is a bit relative, as the plugin doesn't use the DUChain for any semantic analysis -- it's (almost) purely for visualization.
AFAICT this is just kdev-clang importing whatever clang reports with clang_getCursorSemanticParent. Since kdev-clang does have a special code-path for importing base classes as well, inheritance wouldn't be broken by simply not doing that anymore. It might break something else, but the tests should say one way or another. It's worth a shot I think, because imports make lots of lookups more expensive.
Comment Actions
Well, I did not check the other proposed solution of stopping importing the semantic parent. That could fix this issue, but in my opinion would be more of a hack, unless the importing stuff should really be a synonym for inheritance.
I could try to make a change in kdev-clang and see how that performs.
Comment Actions
I've now pushed a proper fix for this issue to kdev-clang. No DUChain feature needs to be changed, it was a misuse of that API that triggered this issue. Olivier was right btw, we simply should not import the semantic parent context if it equals the lexical parent.
commit f945e0f03ee5bc49a0728e4bd7fdd239351745cc
Author: Milian Wolff <mail@milianw.de>
Date: Sun Feb 14 18:38:00 2016 +0100
Don't import the semantic parent if it equals the lexical parent. We to import the semantic parent only if it differs from the lexical parent, as then we deal with out-of-line declarations. For the other case, the normal DUChain tree works fine. And importing the parent again can lead to subtle differences, like DUChainUtils::getInheriters misbehaving for nested classes. This change should also have a nice positive performance impact.
Comment Actions
I have tested these changes (btw: I had to compile 5.0 branch instead of master, it's a bit confusing that the newest fixes are firstly introduced on a release branch and then merged into master from time to time).
It indeed fixes the test case, but importing the semantic parent in case it's different from the lexical parent is still not a good idea. Just moving the inner class definition outside of the outer class reintroduces the problem, i.e. the inner class is claimed to be the inheritor of the outer class.
class X { class Y; };
class X::Y {};
We could remove this importing altogether, with hope that it won't break anything else.
Comment Actions
Thanks for testing, I'll add your code as another test case and look at it. Afaik the import is required for the out-of-line definitions of methods, but maybe it's not required for classes. But as I said, I'll test it.
Regarding the operational way of 5.0 vs. master: We are trying to make sure that 5.0 is polished first, thus fixes such as the one here must go there first. master is open for new big changes or features, but these must not hold up a 5.0 release.
Comment Actions
I've removed the import for nested classes now. Didn't break any tests, and fixes your issue. Thanks for the tests again!