PLEASE REVIEW: modularize: new preprocessor conditional directive checking feature

[Sean Silva]

Hi John, here are some mostly stylistic comments:

+void ModularizeHeaderTracker::AddHeaderFile(
+ ModularizeHeaderFile *headerFile, std::string topHeader) {
+  ModularizeHeaderInstanceVectorIterator iter = HeaderInstances.begin();
+  ModularizeHeaderInstanceVectorIterator EI = HeaderInstances.end();
+  for (; iter != EI; ++iter) {
+    ModularizeHeaderFile *existingHeader = (*iter)->GetHeaderFile();
+    if (existingHeader->Match(headerFile)) {
+  (*iter)->AddTopHeader(topHeader);
+  return;
+  }

Please follow our naming conventios <
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly>
and indentation conventions (2 space soft tabs; you may also find
clang-format useful).

+#if _MSC_VER >= 1400  // VC 8.0 and later deprecate snprintf and _snprintf.
+# define snprintf _snprintf_s
+#elif _MSC_VER
+# define snprintf _snprintf
+#endif

Avoid preprocessor conditionals except in libSupport. You shouldn't be
using this function anyway, really (see below).

+std::string ModularizePPDirective::PrintToString(
+    int level, std::string fileName) {
+  char buffer[1024];
+  char lineBuffer[80];
+  snprintf(lineBuffer, sizeof(lineBuffer) - 1, ":%d:1", LineNumber);
+  strncpy(buffer, Indent(level).c_str(), sizeof(buffer) - 1);
+  strncat(buffer, fileName.c_str(), sizeof(buffer) - 1);
+  strncat(buffer, lineBuffer, sizeof(buffer) - 1);
+  strncat(buffer, ": ", sizeof(buffer) - 1);

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.

+// Do macro substitutions in a string.
+// FIXME: Note that this is very simple parser, such that it doesn't
+// support things like function-style macros, macros with string
+// or character literals, and defined().
+// FIXME: There probably should be a function in the Preprocessor
+// class that will lex a string, do substitutions, and return either
+// a token list or a converted string.
+std::string ModularizePPCallbacks::DoMacroSubstitutions(std::string str) {

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.

+#if MPPC_TRACE
+  Trace(Loc, "Ifndef(%s, %s) called.\n",
+    macroName.c_str(), (MD ? " (defined)" : " (not defined)"));
+  Trace("CurrentHeaderFile.Name = %s\n",
CurrentHeaderFile->GetName().c_str());
+#endif

We have the DEBUG macro (and friends) which can use used for this purpose.

+  /// \brief Hook called whenever an \#elif is seen.
+  /// \param Loc the source location of the directive.
+  /// \param ConditionRange The SourceRange of the expression being tested.
+  /// \param IfLoc the source location of the \#if/\#ifdef/\#ifndef
directive.
+  // FIXME: better to pass in a list (or tree!) of Tokens.
+  void Elif(SourceLocation Loc, SourceRange ConditionRange,
+                    SourceLocation IfLoc);

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.

+#if 0 // Can result in mismatched files.
+  ModularizeHeaderFile *mhf = NULL;
+  mhf = GetHeaderFile(fileName);
+  if (mhf == NULL) {
+    mhf = new ModularizeHeaderFile(fileName, PP);
+    AddHeaderFile(mhf);
+    CurrentHeaderFile = mhf;
+    if (!RootHeaderFile)
+      RootHeaderFile = mhf;
+  }
+  else
+    CurrentHeaderFile = mhf;
+#endif

Don't leave in disabled code.

-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130611/e448cb57/attachment.html>