[PATCH] D15410: AnalysisConsumer: use canonical decl for both lookup and store of visited decls

[Anna Zaks via cfe-commits]

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