Review Request 119526: Ensure we're not visiting cursors twice

[Olivier Jean de Gaalon]

> On July 29, 2014, 5:11 a.m., Olivier Jean de Gaalon wrote:
> > I would prefer to look into this issue deeper, adding a hash of visited cursors isn't really an acceptable solution IMO.
> > 
> > Also not sure what is meant by "Apparently just 'recursing' vs. 'visiting children explicitly' results in a different AST traversal"
> > 
> > There should be no difference between clang_visitChildren on a cursor vs returning CXChildVisitRecurse. If we are doing both that is a problem, but that doesn't seem to be the case (IIRC we don't ever use CXChildVisitRecurse). On the other hand, returning CXChildVisitContinue means no further recursion is desired (ie, skip the children of this cursor).
> > 
> > This looks like a defect in the clang AST. Looking at it with clang-parser I see no reason for the StructDecl to be a child of the variable declaration. A workaround is still needed, but it should target this pattern without hashing every cursor in the TU, which would really be a last resort.
> 
> Kevin Funk wrote:
>     "There should be no difference between clang_visitChildren on a cursor vs returning CXChildVisitRecurse. If we are doing both that is a problem, but that doesn't seem to be the case (IIRC we don't ever use CXChildVisitRecurse). On the other hand, returning CXChildVisitContinue means no further recursion is desired (ie, skip the children of this cursor)."
>     
>     => Damn, I think that's indeed the case. I was playing around with DebugVisitor, removed the call to `clang_visitChildren` but forgot to make it return `CXChildVisit_Recurse` instead. So, indeed, even with `CXChildVisit_Recurse` the traversal is the same.
>     
>     The issue still remains. We're visiting some cursors multiple times. I also don't really see why the StructDecl needs to be a child of the VarDecl. Maybe worth asking on the mailing list -- I'll do that.
>     
>     I think it's still sane to have this committed for now (with a big-fat TODO), otherwise we and up duplicating decls and types in the DUChain.

We don't need to pessimize every single cursor to introduce a hack for this case. Maybe try specializing dispatchCursor<CXCursor_StructDecl> and skipping it if the parent is a VarDecl (wrapped in the big-fat HACK/TODO markers of course). If needed you can even pass the parent in to dispatchCursor and update the switch.


- Olivier Jean de


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119526/#review63378
-----------------------------------------------------------


On July 28, 2014, 8:25 p.m., Kevin Funk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119526/
> -----------------------------------------------------------
> 
> (Updated July 28, 2014, 8:25 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> Ensure we're not visiting cursors twice
> 
> 
> Diffs
> -----
> 
>   duchain/tuduchain.h 00ee23ba0da851456fd2a2641689514d25240389 
>   duchain/tuduchain.cpp a9e249e16a1d82c95c360b40575568abc1311444 
>   tests/test_duchain.h 37ce20f398ea73d2a36d753a6421bb3015898f4d 
>   tests/test_duchain.cpp 89f5005149421b8b3ef1a9def3d9e6222e3d6b0a 
> 
> Diff: https://git.reviewboard.kde.org/r/119526/diff/
> 
> 
> Testing
> -------
> 
> Please check, maybe I'm missing something.
> 
> 
> Thanks,
> 
> Kevin Funk
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140729/8369773a/attachment-0001.html>