[PATCH] [extra] pptrace - preprocessor tracing and testing tool

[Kim Gräsman]

  I'm in no position to sign off commits, but this looks good and useful to me. A few small comments/questions, otherwise I'm happy. Thanks!


================
Comment at: pp-trace/PPCallbacksTracker.h:218
@@ +217,3 @@
+  /// \brief Append a double-quoted argument to the top trace item.
+  void appendQuotedArgument(const char *Name, std::string &Value);
+
----------------
Why not a const reference?

================
Comment at: pp-trace/PPCallbacksTracker.cpp:564-591
@@ +563,30 @@
+  std::string Str;
+  llvm::raw_string_ostream SS(Str);
+  SS << "[";
+  // The argument tokens might include end tokens, so we reflect how
+  // how getUnexpArgument provides the arguments.
+  for (int I = 0, E = Value->getNumArguments(); I < E; ++I) {
+    const clang::Token *Current = Value->getUnexpArgument(I);
+    int TokenCount = Value->getArgLength(Current) + 1; // include EOF
+    E -= TokenCount;
+    if (I)
+      SS << ", ";
+    // We're assuming tokens are contiguous, as otherwise we have no
+    // other way to get at them.
+    --TokenCount;
+    for (int TokenIndex = 0; TokenIndex < TokenCount; ++TokenIndex, ++Current) {
+      if (TokenIndex)
+        SS << " ";
+      // We need to be careful here because the arguments might not be legal in
+      // YAML, so we use the token name for anything but identifiers and
+      // numeric literals.
+      if (Current->isAnyIdentifier() ||
+          Current->is(clang::tok::numeric_constant)) {
+        SS << PP.getSpelling(*Current);
+      } else {
+        SS << "<" << Current->getName() << ">";
+      }
+    }
+  }
+  SS << "]";
+  appendArgument(Name, SS.str());
----------------
Nice! :-)

================
Comment at: pp-trace/PPCallbacksTracker.cpp:607
@@ +606,3 @@
+void PPCallbacksTracker::appendQuotedArgument(const char *Name,
+                                              std::string &Value) {
+  std::string Str;
----------------
const reference?

================
Comment at: pp-trace/PPTrace.cpp:109
@@ +108,3 @@
+      : CallbacksTracker(new PPCallbacksTracker(Ignore, CallbackCalls, PP)) {
+    PP.addPPCallbacks(CallbacksTracker); // Takes ownership.
+  }
----------------
Just pass callbacks tracker into addPPCallbacks immediately:

  PP.addPPCallbacks(new PPCallbacksTracker(Ignore, CallbackCalls, PP));

================
Comment at: pp-trace/PPTrace.cpp:112-113
@@ +111,4 @@
+
+private:
+  PPCallbacksTracker *CallbacksTracker; // Not owned here.
+};
----------------
No use keeping this as a member

================
Comment at: pp-trace/PPTrace.cpp:210-211
@@ +209,4 @@
+  // Do the output.
+  // If file name is "-", output to stdout.
+  if (!OutputFileName.size() || (OutputFileName == "-")) {
+    HadErrors = outputPPTrace(CallbackCalls, llvm::outs());
----------------
Why is the "-" necessary? I just leave out -output and pp-trace dutifully prints to stdout.


http://llvm-reviews.chandlerc.com/D2020