<div dir="ltr"><div>There are still a couple naming issues, e.g.</div><div><br></div><div><div>+    PPItemKey instanceKey(PP, MacroName, H, InstanceLoc);</div><div>+    PPItemKey definitionKey(PP, MacroName, H, DefinitionLoc);</div>
</div><div><br></div><div><div>+    StringHandle conditionUnexpanded(addString(ConditionUnexpanded));</div><div>+    PPItemKey instanceKey(PP, conditionUnexpanded, H, InstanceLoc);</div></div><div><br></div><div>+        std::string definitionSourceLine =<br>
</div><div><br></div><div>+    bool returnValue = false;<br></div><div><br></div><div>Make sure to hammer these out.</div><div><br></div><div><div>+///</div><div>+/// \file</div><div>+/// \brief Track preprocessor activities for modularize.</div>
<div>+///</div></div><div><br></div><div>You should probably describe the things that it tracks and why it tracks them.</div><div><br></div><div><div>+// Forwards.</div><div>+class PreprocessorTrackerImpl;</div><div>+</div>
<div>+// Some handle types</div><div>+</div><div>+// String handle.</div><div>+typedef llvm::PooledStringPtr StringHandle;</div><div>+</div><div>+// Header handle.</div><div>+typedef int HeaderHandle;</div><div>+const HeaderHandle HeaderHandleInvalid = -1;</div>
<div>+</div><div>+// Header inclusion path handle.</div><div>+typedef int InclusionPathHandle;</div><div>+const InclusionPathHandle InclusionPathHandleInvalid = -1;</div></div><div><br></div><div>These comments are unnecessary.</div>
<div><br></div><div>Also, I'd like to see the overall architecture reflected in a file-level comment somewhere.</div><div><br></div><div>+  // Overidden handlers.<br></div><div><br></div><div>Spelling. Overidden -> Overriden</div>
<div><br></div><div>-- Sean Silva</div></div>