zaks.anna added a comment.
I wonder if we can refactor the code so that it is less error prone..
shouldSkipFunction(D, Visited, VisitedAsTopLevel) works with two sets. I assume
that you have not updated Decls coming from VisitedAsTopLevel because they come
from the CFG and should already be canonical. In this patch, you make sure the
values stored in Visited are canonical inside ExprEngineCallAndReturn.cpp.
Unfortunately, now the conversion to canonical form is spread all over, which
makes it error prone.
Note also that the conversion into canonical from inside the CallGraph is
different than what you have:
if (F && !isa<ObjCMethodDecl>(F))
F = F->getCanonicalDecl();
Here is a copy an paste from the review of the patch that introduced that code
that attempts to explain the special handling of ObjC:
"This differs from what we discussed by not using the canonical declaration for
ObjCMethodDecls. This is because hasBody and getBody work differently for
ObjCMethodDecls than FunctionDecls. Using the canonical declaration breaks the
call graph. There isn't a problem with the call graph created for
ObjCMethodDecls, so I have left them alone."
I suggest:
- Try to figure out what's special about ObjCMethodDecl and can that be fixed
in some other way.
- Move the getCanonicalDecl conversion into HandleDeclsCallGraph() + maybe add
a comment saying why the decls inside VisitedAsTopLevel are canonical already.
This way most of the code would be in one place. Would that work?
Repository:
rL LLVM
http://reviews.llvm.org/D15410
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
