<div dir="ltr">Hi John, here are some mostly stylistic comments:<div><br></div><div><div>+void ModularizeHeaderTracker::AddHeaderFile(</div><div>+<span class="" style="white-space:pre">   </span>ModularizeHeaderFile *headerFile, std::string topHeader) {</div>
<div>+  ModularizeHeaderInstanceVectorIterator iter = HeaderInstances.begin();</div><div>+  ModularizeHeaderInstanceVectorIterator EI = HeaderInstances.end();</div><div>+  for (; iter != EI; ++iter) {</div><div>+    ModularizeHeaderFile *existingHeader = (*iter)->GetHeaderFile();</div>
<div>+    if (existingHeader->Match(headerFile)) {</div><div>+<span class="" style="white-space:pre">            </span>  (*iter)->AddTopHeader(topHeader);</div><div>+<span class="" style="white-space:pre">            </span>  return;</div>
<div>+<span class="" style="white-space:pre">   </span>  }</div></div><div><br></div><div style>Please follow our naming conventios <<a href="http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly">http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly</a>> and indentation conventions (2 space soft tabs; you may also find clang-format useful).</div>
<div style><br></div><div style><div>+#if _MSC_VER >= 1400  // VC 8.0 and later deprecate snprintf and _snprintf.</div><div>+# define snprintf _snprintf_s</div><div>+#elif _MSC_VER</div><div>+# define snprintf _snprintf</div>
<div>+#endif</div><div><br></div><div style>Avoid preprocessor conditionals except in libSupport. You shouldn't be using this function anyway, really (see below).</div><div style><br></div><div style><div>+std::string ModularizePPDirective::PrintToString(</div>
<div>+    int level, std::string fileName) {</div><div>+  char buffer[1024];</div><div>+  char lineBuffer[80];</div><div>+  snprintf(lineBuffer, sizeof(lineBuffer) - 1, ":%d:1", LineNumber);</div><div>+  strncpy(buffer, Indent(level).c_str(), sizeof(buffer) - 1);</div>
<div>+  strncat(buffer, fileName.c_str(), sizeof(buffer) - 1);</div><div>+  strncat(buffer, lineBuffer, sizeof(buffer) - 1);</div><div>+  strncat(buffer, ": ", sizeof(buffer) - 1);</div><div><br></div><div style>
Use raw_ostream for this instead of error-prone C-style string manipulation. raw_svector_ostream or raw_string_ostream are a good fit for this.</div></div></div><div style><br></div><div style><div>+// Do macro substitutions in a string.</div>
<div>+// FIXME: Note that this is very simple parser, such that it doesn't</div><div>+// support things like function-style macros, macros with string</div><div>+// or character literals, and defined().</div><div>+// FIXME: There probably should be a function in the Preprocessor</div>
<div>+// class that will lex a string, do substitutions, and return either</div><div>+// a token list or a converted string.</div><div>+std::string ModularizePPCallbacks::DoMacroSubstitutions(std::string str) {</div><div>
<br></div><div style>I'm pretty sure that Clang has functionality to do this. Please don't invent your own. If you need to add functionality to clang's Preprocessor, please do so in a separate patch.</div><div style>
<br></div><div style><div>+#if MPPC_TRACE</div><div>+  Trace(Loc, "Ifndef(%s, %s) called.\n",</div><div>+    macroName.c_str(), (MD ? " (defined)" : " (not defined)"));</div><div>+  Trace("CurrentHeaderFile.Name = %s\n", CurrentHeaderFile->GetName().c_str());</div>
<div>+#endif</div><div><br></div><div style>We have the DEBUG macro (and friends) which can use used for this purpose.</div><div style><br></div><div style><div>+  /// \brief Hook called whenever an \#elif is seen.</div><div>
+  /// \param Loc the source location of the directive.</div><div>+  /// \param ConditionRange The SourceRange of the expression being tested.</div><div>+  /// \param IfLoc the source location of the \#if/\#ifdef/\#ifndef directive.</div>
<div>+  // FIXME: better to pass in a list (or tree!) of Tokens.</div><div>+  void Elif(SourceLocation Loc, SourceRange ConditionRange,</div><div>+                    SourceLocation IfLoc);</div><div><br></div><div style>
Please don't just copy the documentation from PPCallbacks. You may want to discuss what this particular concrete subclass is doing in the callbacks though.</div><div style><br></div><div style><div>+#if 0 // Can result in mismatched files.</div>
<div>+  ModularizeHeaderFile *mhf = NULL;</div><div>+  mhf = GetHeaderFile(fileName);</div><div>+  if (mhf == NULL) {</div><div>+    mhf = new ModularizeHeaderFile(fileName, PP);</div><div>+    AddHeaderFile(mhf);</div><div>
+    CurrentHeaderFile = mhf;</div><div>+    if (!RootHeaderFile)</div><div>+      RootHeaderFile = mhf;</div><div>+  }</div><div>+  else</div><div>+    CurrentHeaderFile = mhf;</div><div>+#endif</div><div><br></div><div style>
Don't leave in disabled code.</div></div><div style><br></div></div></div><div style>-- Sean Silva</div></div></div>