[PATCH] cpp11-migrate: Add a class to support include directives modifications

[Edwin Vane]

  With respect to clang-tidy, should this code live in clang::tooling?


================
Comment at: cpp11-migrate/Core/IncludeDirectives.h:11
@@ +10,3 @@
+/// \file
+/// \brief This file declares the IncludeDirectives class that helps detecting
+/// and modifying \#include directives.
----------------
...helps **with** detecting...

================
Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:1
@@ +1,2 @@
+//===-- Core/IncludeDirectives.h - Include directives handling ------------===//
+//
----------------
Nit: this is the .cpp file not .h.

================
Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:28
@@ +27,3 @@
+/// \c IncludeDirectives.
+class IncludeDirectivesPPCallback : public clang::PPCallbacks {
+public:
----------------
Any reason this callback class isn't also in the anonymous namespace?

================
Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:55
@@ +54,3 @@
+
+namespace {
+// Guess the end-of-line sequence used in the given FileID. If the sequence
----------------
Maybe it's just me but I like all the anonymous namespace stuff at the top of the file and not interspersed between the implementations of member functions for a class.

================
Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:52
@@ +51,3 @@
+    : CI(CI), Sources(CI.getSourceManager()) {
+  CI.getPreprocessor().addPPCallbacks(new IncludeDirectivesPPCallback(this));
+}
----------------
Maybe a quick comment to indicate addPPCallbacks() takes ownership of the provided callback.

================
Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:62
@@ +61,3 @@
+  StringRef Buffer;
+  if (!Invalid)
+    Buffer = Content.substr(Content.find_first_of("\r\n"));
----------------
I would just `assert(!Invalid)`.

================
Comment at: cpp11-migrate/Core/IncludeDirectives.h:82
@@ +81,3 @@
+  typedef llvm::SmallPtrSet<const clang::FileEntry *, 32> SeenFilesSet;
+  bool lookForInclude(const clang::FileEntry *File, llvm::StringRef Include,
+                      const LocationVec &Locations, SeenFilesSet &Seen) const;
----------------
I know private member functions don't require documentation but a little bit might help with these functions since their parameter lists are non-trivial.

================
Comment at: cpp11-migrate/Core/IncludeDirectives.h:66
@@ +65,3 @@
+    clang::SourceLocation Loc;
+    const clang::FileEntry *File;
+    bool Angled;
----------------
Is this member necessary? Scanning the uses of this class it seems this information is already available elsewhere.

================
Comment at: unittests/cpp11-migrate/IncludeDirectivesTest.cpp:165
@@ +164,3 @@
+TEST(IncludeDirectivesTest, indirectIncludes) {
+  EXPECT_EQ(
+      "#include </virtual/foo.h>\n",
----------------
Isnt' this test the same as avoidDuplicates?

================
Comment at: cpp11-migrate/Core/IncludeDirectives.cpp:233
@@ +232,3 @@
+  Token Tok;
+  for (;;) {
+    // find beginning of end-of-line sequence
----------------
Maybe a comment about what this for loop is trying to accomplish. Some examples would be nice.


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