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

Sean Silva silvas at purdue.edu
Thu Oct 24 18:12:54 PDT 2013


  I don't see the purpose of the "generic" output format. It is only 1 character away from being valid YAML. The following is valid YAML (Assuming the ArgumentK values are unique, which I think is true in this case):

  ```
  - CallbackName:
      Argument1: Value1
      Argument2: Value2
  ```

  The overhead of maintaining two output formats doesn't seem worth it. For simplicity of code that manipulates this output, it may be best to use something like:

  ```
  - Kind: CallbackName
    Argument1: Value1
    Argument2: Value2
  ```

  (i.e., have the callback name just be another field of the record; this is a very common pattern for handling serialized variant data in dynamic languages (and YAML's semantic model is basically wed to dynamic languages)).

  Since you don't need to read this information back into the same data structure that produced it, it seems like it might be simpler to forego YAMLIO.


================
Comment at: pp-trace/PPCallbacksTracker.cpp:625
@@ +624,3 @@
+    return;
+  char Buffer[256];
+  va_list Args;
----------------
Ew. No. Fixed size buffers are evil.

It seems like most of the reason you need this is just to print a string "Foo(Bar)". Better to just have a subroutine taking the two strings "Foo" and "Bar" and using raw_string_ostream or raw_svector_ostream to generate that output.

================
Comment at: pp-trace/PPCallbacksTracker.cpp:606-607
@@ +605,4 @@
+    return;
+  }
+  appendArgumentFormatted(Name, "(%d)", Value->getNumArguments());
+}
----------------
This output doesn't seem very useful.

================
Comment at: pp-trace/PPCallbacksTracker.cpp:176
@@ +175,3 @@
+                                      const clang::Module *Imported) {
+  beginCallback("moduleImport");
+  appendArgument("ImportLoc", ImportLoc);
----------------
Any particular reason why the capitalization for this is different from the rest?

================
Comment at: pp-trace/PPCallbacksTracker.cpp:20
@@ +19,3 @@
+#include <stdio.h>
+#include "PPCallbacksTracker.h"
+
----------------
<http://llvm.org/docs/CodingStandards.html#include-style>. In particular, "PPCallbacksTracker.h" should be first.

================
Comment at: pp-trace/PPCallbacksTracker.h:118-154
@@ +117,39 @@
+
+  /// \brief Callback invoked whenever an inclusion directive of
+  /// any kind (\c \#include, \c \#import, etc.) has been processed, regardless
+  /// of whether the inclusion will actually result in an inclusion.
+  ///
+  /// \param HashLoc The location of the '#' that starts the inclusion
+  /// directive.
+  ///
+  /// \param IncludeTok The token that indicates the kind of inclusion
+  /// directive, e.g., 'include' or 'import'.
+  ///
+  /// \param FileName The name of the file being included, as written in the
+  /// source code.
+  ///
+  /// \param IsAngled Whether the file name was enclosed in angle brackets;
+  /// otherwise, it was enclosed in quotes.
+  ///
+  /// \param FilenameRange The character range of the quotes or angle brackets
+  /// for the written file name.
+  ///
+  /// \param File The actual file that may be included by this inclusion
+  /// directive.
+  ///
+  /// \param SearchPath Contains the search path which was used to find the file
+  /// in the file system. If the file was found via an absolute include path,
+  /// SearchPath will be empty. For framework includes, the SearchPath and
+  /// RelativePath will be split up. For example, if an include of "Some/Some.h"
+  /// is found via the framework path
+  /// "path/to/Frameworks/Some.framework/Headers/Some.h", SearchPath will be
+  /// "path/to/Frameworks/Some.framework/Headers" and RelativePath will be
+  /// "Some.h".
+  ///
+  /// \param RelativePath The path relative to SearchPath, at which the include
+  /// file was found. This is equal to FileName except for framework includes.
+  ///
+  /// \param Imported The module, whenever an inclusion directive was
+  /// automatically turned into a module import or null otherwise.
+  ///
+  virtual void InclusionDirective(clang::SourceLocation HashLoc,
----------------
Don't copy the documentation from the base class.

================
Comment at: pp-trace/PPCallbacksTracker.h:155-162
@@ +154,10 @@
+  ///
+  virtual void InclusionDirective(clang::SourceLocation HashLoc,
+                                  const clang::Token &IncludeTok,
+                                  llvm::StringRef FileName, bool IsAngled,
+                                  clang::CharSourceRange FilenameRange,
+                                  const clang::FileEntry *File,
+                                  llvm::StringRef SearchPath,
+                                  llvm::StringRef RelativePath,
+                                  const clang::Module *Imported);
+
----------------
Use `LLVM_OVERRIDE` (i.e. C++11 `override`) instead of prefixing `virtual` to all these overriden virtual functions. That way, C++11 builds (which many people are doing these days) will benefit from the error checking.


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



More information about the cfe-commits mailing list