In http://reviews.llvm.org/D9498#175201, @rsmith wrote:
> Essentially this seems fine. It'd be better if `visitDepthFirst` didn't
> actually walk the the entire module graph in the case where it's been told to
> `SkipChildren`, but we can leave that improvement for a later change.
I'm not sure how that would be possible due to imports building a DAG.
The trade-off seems to be: either we remember the knowledge that all transitive
deps do not need to be visited, or we might have to re-calculate that
information when we come in via a different edge. My experiments indicate that
the latter is common enough to offset the cost of the former.
================
Comment at: include/clang/Serialization/ModuleManager.h:295-296
@@ -283,4 +294,4 @@
/// which will be passed along to the user.
- void visitDepthFirst(bool (*Visitor)(ModuleFile &M, bool Preorder,
- void *UserData),
+ void visitDepthFirst(bool (*PostorderVisitor)(ModuleFile &M, void *UserData),
+ DFSPreorderControl (*PreorderVisitor)(ModuleFile &M,
void *UserData),
void *UserData);
----------------
rsmith wrote:
> These parameters seem backwards to me (on each node, we'll call the preorder
> visitor before the postorder one, so it would seem more natural for the
> preorder one to be written first in a call to this function).
... says the person whose trees grow upside-down ;) Done.
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:3363
@@ +3362,3 @@
+
+ bool needsToVisitChildren(ModuleFile &M, GlobalDeclID GlobalID) {
+ DeclID ID = Reader.mapGlobalIDToModuleFileGlobalID(M, GlobalID);
----------------
rsmith wrote:
> `Children` here is ambiguous, and entirely backwards from the way I think of
> the imports graph (where the roots import nothing and the leaves are not
> imported by anything). Calling this `Imports` would remove the ambiguity
> (likewise in the `DFSPreorderControl` enum).
Note that there is precedence in the ModuleManager.h of calling the imports
"children" :P
Your trees grow the wrong way (granted, this isn't *really* a tree).
On the other hand, "imports" is clearly the better name, so Done :D
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:3376-3378
@@ +3375,5 @@
+ }
+ unsigned Offset = Result->Offset;
+ unsigned N = M.RedeclarationChains[Offset];
+ return N != 0;
+ }
----------------
rsmith wrote:
> A comment explaining what's going on here ("we don't need to visit a module
> or any of its imports if we've already deserialized the redecls from this
> module") would help.
Done.
http://reviews.llvm.org/D9498
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
