[PATCH] cpp11-migrate: Header change application tool - migmerge

[Guillaume Papin]

  The tool looks nice, looking forward the header migration.

  Ideas for the future, not sure it's the right place:
  - Do you think about skipping other directories than ".git"? A regex might be better (or a list of directory). For example a grep like tool name Ack ignore the following directories: https://github.com/petdance/ack/blob/master/Ack.pm#L50

  Your demo made me think about something.
  When you used `cat *yaml` I realized that the way the serialization is done we could concat the YAML documents generated by a transformation in only one file. See the comment about multiple document at the end of the example here: http://en.wikipedia.org/wiki/YAML#Sample_document. That's something not possible with JSON where arrays end with `}`.

  If we happen to have one tool that calls the migrator on a tree and then merge headers for us then I think we could put the YAML serialization of the replacements in a file named after the headers, for example:

  For a/header.h generating a file a/header.h.replacements:
  ```
    ---
    <transformation results>
    ...
    ---
    <another transformation results>
    ...
    <...>
  ```


================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.h:21-22
@@ +20,4 @@
+
+// Forward declarations
+
+namespace clang {
----------------
Nit: usually there is no blank line after this comment.

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.h:27
@@ +26,3 @@
+
+bool applyChangeDescriptions(const llvm::StringRef Directory,
+                             clang::DiagnosticsEngine &Diagnostics);
----------------
Any reason for the missing Doxygen?

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.h:11-12
@@ +10,4 @@
+/// \file
+/// \brief This file provides the declaration for the base Transform class from
+/// which all transforms must subclass.
+///
----------------
Copy & paste error.

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:1
@@ +1,2 @@
+#include "Core/ReplacementsYaml.h"
+
----------------
Missing file header.

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:5
@@ +4,3 @@
+#include "clang/Basic/SourceManager.h"
+
+#include "llvm/Support/FileSystem.h"
----------------
Most of the group of includes seems to be separated by a blank line, I think you or someone else told me not to do so.

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:10-11
@@ +9,4 @@
+#include "llvm/Support/raw_ostream.h"
+
+
+using namespace llvm;
----------------
clang-format will remove the extra-line here.

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:18
@@ +17,3 @@
+
+void eatDiagnostics(const SMDiagnostic &, void *) {}
+
----------------
static?

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:20
@@ +19,3 @@
+
+error_code collectHeaderChangeDocs(const StringRef Directory,
+                                   HeaderChangeDocs &ChangeDocs,
----------------
static?

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:30-31
@@ +29,4 @@
+       I != E && !ErrorCode; I.increment(ErrorCode)) {
+    StringRef Filename(filename(I->path()));
+    if (Filename == ".git") {
+      I.no_push();
----------------
`if (filename(I->path()) == ".git")` seems clear enough to me (no other use of Filename apparently.

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:32
@@ +31,3 @@
+    if (Filename == ".git") {
+      I.no_push();
+      continue;
----------------
Maybe a comment around this code explaining what it does.

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:40
@@ +39,3 @@
+    OwningPtr<MemoryBuffer> Out;
+    error_code BufferError= MemoryBuffer::getFile(I->path(), Out);
+    if (BufferError) {
----------------
missing space.

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:47
@@ +46,3 @@
+
+    yaml::Input yin(Out->getBuffer());
+    yin.setDiagHandler(&eatDiagnostics);
----------------
yin -> YIn/Yin (coding standard says: "The name should be camel case, and start with an upper case letter.").

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:73-74
@@ +72,4 @@
+
+  error_code ErrorCode;
+  ErrorCode = collectHeaderChangeDocs(Directory, Docs, FoundFiles);
+
----------------
Why not:

  error_code ErrorCode = collectHeaderChangeDocs(Directory, Docs, FoundFiles);

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:93-94
@@ +92,4 @@
+       I != E; ++I) {
+    GroupedReplacements[I->HeaderFileName]
+        .insert(GroupedReplacements[I->HeaderFileName].end(),
+                I->Replacements.begin(), I->Replacements.end());
----------------
Maybe avoid a lookup in the map by retrieving `GroupedReplacements[I->HeaderFileName]` only once.

================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:115
@@ +114,3 @@
+    std::vector<tooling::Range> Conflicts;
+    deduplicate(I->getValue(), Conflicts);
+
----------------
I'm surprised, `deduplicate()` is in `tooling::` no? I don't see any `using tooling` or similar.

================
Comment at: cpp11-migrate/tool/CMakeLists.txt:46-48
@@ +45,5 @@
+
+set(LLVM_OPTIONAL_SOURCES
+  Cpp11Migrate.cpp
+  )
+
----------------
Why that?
Not sure about I'm right here but I believe with the way CMake lists works if you want both Cpp11Migrate.cpp and MergeMain.cpp to be optional you should append Cpp11Migrate.cpp to the list using:

  set(LLVM_OPTIONAL_SOURCES
    Cpp11Migrate.cpp
    ${LLVM_OPTIONAL_SOURCES}
    )

And maybe a comment with a reason it's optional.

================
Comment at: cpp11-migrate/tool/MergeMain.cpp:13
@@ +12,3 @@
+Directory(cl::Positional, cl::Required,
+          cl::desc("<Search Root Directory>"));
+
----------------
I think the angles aren't needed.


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