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

[Kim Gräsman]

  Ah, I completely missed the fact that your code was calling the asserting method. Works fine now, thanks!

  Bunch of aesthetic comments added, I'll try this on some bigger preprocessor challenges...


================
Comment at: pp-trace/PPCallbacksTracker.h:22-23
@@ +21,4 @@
+
+#ifndef PPTRACE_CALLBACS_H
+#define PPTRACE_CALLBACS_H
+
----------------
Should be PPTRACE_CALLBACKS_H or maybe match the filename in full, i.e. PPTRACE_PPCALLBACKSTRACKER_H?

================
Comment at: pp-trace/PPCallbacksTracker.h:70
@@ +69,3 @@
+public:
+  /// \brief PPCallbacksTracker constructor.
+  /// \param Ignore - Set of names of callbacks to ignore.
----------------
Style guide says to avoid repeating class/function names in \brief. Not sure what to say about the ctor in \brief, maybe something to the effect of ownership?

================
Comment at: pp-trace/PPCallbacksTracker.cpp:1
@@ +1,2 @@
+//===--- PPCallbacksTracker.cpp - Preprocessor tracker -*- C++ -*--------===//
+//
----------------
You don't need *- C++ -* in .cpp files.

================
Comment at: pp-trace/PPCallbacksTracker.cpp:81
@@ +80,3 @@
+// Mapping strings.
+static const char *MappingStrings[] = { "0",           "MAP_IGNORE",
+                                        "MAP_WARNING", "MAP_ERROR",
----------------
Indentation looks funny here, but maybe that's a good thing considering that "0" is apparently not in the enum. :-)

================
Comment at: pp-trace/PPCallbacksTracker.cpp:87-90
@@ +86,6 @@
+
+// Constructor.
+// Trace - The trace buffer.  One string per callback.
+// IgnoreCallbacks - Names of callbacks to ignore. Null terminates the list.
+// PP - The preprocessor.  Needed for getting some argument strings.
+PPCallbacksTracker::PPCallbacksTracker(llvm::SmallSet<std::string, 4> &Ignore,
----------------
This duplicates the information from the header. I vote to remove it.

================
Comment at: pp-trace/PPCallbacksTracker.cpp:96-97
@@ +95,4 @@
+
+// Destructor.
+PPCallbacksTracker::~PPCallbacksTracker() {}
+
----------------
The comment seems redundant to me.

================
Comment at: pp-trace/PPCallbacksTracker.cpp:101
@@ +100,3 @@
+
+// Callback invoked whenever a source file is entered or exited.
+void PPCallbacksTracker::FileChanged(
----------------
Should these be doxygen comments?

================
Comment at: pp-trace/PPCallbacksTracker.cpp:212
@@ +211,3 @@
+                                     llvm::StringRef DebugType) {
+  beginCallback("PragmaDetectMismatch");
+  appendArgument("Loc", Loc);
----------------
PragmaDebug

================
Comment at: pp-trace/PPCallbacksTracker.cpp:251
@@ +250,3 @@
+                                          llvm::StringRef Str) {
+  beginCallback("PragmaDiagnosticPop");
+  appendArgument("Loc", Loc);
----------------
PragmaDiagnostic

================
Comment at: pp-trace/PPCallbacksTracker.cpp:281
@@ +280,3 @@
+  SS << "[";
+  for (int i = 0, e = Ids.size(); i != e; ++i) {
+    if (i)
----------------
I saw Sean suggested lower-case index names, but the coding guidelines seem to advocate upper-case I and E.

================
Comment at: pp-trace/PPCallbacksTracker.cpp:311
@@ +310,3 @@
+  appendArgument("MacroNameTok", MacroNameTok);
+  appendArgument("MacroDirective", MD);
+  appendArgument("Range", Range);
----------------
I'm not sure what to think about naming the argument in the output something else than the actual arg. It stands out, but it would be nice if the MD argument was in fact named MacroDirective (though it clashes with the type...)

================
Comment at: pp-trace/PPCallbacksTracker.cpp:321
@@ +320,3 @@
+  appendArgument("MacroNameTok", MacroNameTok);
+  appendArgument("MacroDirective", MD);
+}
----------------
MacroDirective/MD

================
Comment at: pp-trace/PPCallbacksTracker.cpp:329
@@ +328,3 @@
+  appendArgument("MacroNameTok", MacroNameTok);
+  appendArgument("MacroDirective", MD);
+}
----------------
MacroDirective/MD

================
Comment at: pp-trace/PPCallbacksTracker.cpp:338
@@ +337,3 @@
+  appendArgument("MacroNameTok", MacroNameTok);
+  appendArgument("MacroDirective", MD);
+  appendArgument("Range", Range);
----------------
MacroDirective/MD

================
Comment at: pp-trace/PPCallbacksTracker.cpp:377
@@ +376,3 @@
+  appendArgument("MacroNameTok", MacroNameTok);
+  appendArgument("MacroDirective", MD);
+}
----------------
MacroDirective/MD

================
Comment at: pp-trace/PPCallbacksTracker.cpp:387
@@ +386,3 @@
+  appendArgument("MacroNameTok", MacroNameTok);
+  appendArgument("MacroDirective", MD);
+}
----------------
MacroDirective/MD

================
Comment at: pp-trace/PPCallbacksTracker.cpp:410
@@ +409,3 @@
+void PPCallbacksTracker::beginCallback(const char *Name) {
+  DisableTrace = Ignore.count(std::string(Name));
+  if (DisableTrace)
----------------
I'll never get used to the fact that SmallSet::count returns a bool :-)

================
Comment at: pp-trace/PPCallbacksTracker.cpp:443
@@ +442,3 @@
+// Append a string object argument to the top trace item.
+void PPCallbacksTracker::appendArgument(const char *Name, std::string Value) {
+  appendArgument(Name, Value.c_str());
----------------
This should be a const std::string &, right?

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

================
Comment at: pp-trace/PPTrace.cpp:154-166
@@ +153,15 @@
+
+  for (std::vector<CallbackCall>::iterator I = CallbackCalls.begin(),
+                                           E = CallbackCalls.end();
+       I != E; ++I) {
+    CallbackCall &Callback = *I;
+    OS << "- Callback: " << Callback.Name << "\n";
+
+    for (std::vector<Argument>::iterator AI = Callback.Arguments.begin(),
+                                         AE = Callback.Arguments.end();
+         AI != AE; ++AI) {
+      Argument &Arg = *AI;
+      OS << "  " << Arg.Name << ": " << Arg.Value << "\n";
+    }
+  }
+
----------------
I prefer const_iterator and const refs to elements, but I don't know what the general convention for LLVM is.

================
Comment at: pp-trace/PPTrace.cpp:182
@@ +181,3 @@
+  SmallVector<StringRef, 32> IgnoreCallbacksStrings;
+  StringRef(IgnoreCallbacks).split(IgnoreCallbacksStrings, ",", -1, false);
+  SmallSet<std::string, 4> Ignore;
----------------
/*MaxSplit=*/-1, /*KeepEmpty=*/false

================
Comment at: pp-trace/PPTrace.cpp:210-212
@@ +209,5 @@
+  // If file name is "-", output to stdout.
+  if (!OutputFileName.size() || (OutputFileName == "-"))
+    HadErrors = outputPPTrace(CallbackCalls, llvm::outs());
+  else {
+    // Set up output file.
----------------
Add braces around this block to keep symmetry with else


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