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

[Edwin Vane]

  @Sarcasm:
  The .git thing is actually left over from when I copied this code out of [[https://github.com/revane/migmerge_git|migmerge_git]]. Now that you remind me of this fact, I think this tool needs some support for knowing which directories to look in and which to avoid. For now I think I'll skip any directories starting with '.'.

  As for multiple docs per YAML file, we discussed this in the planning meeting this morning but for the benefit of others I'll repeat what was said there. The benefit of concatenating files doesn't really buy us much and would be painful especially when we start doing migrations in parallel with process-based parallelism.

  @klimek:
  While having serialization stuff for replacements in tooling sounds like a great idea I agree it's something to do at a later time. Note that when we serialize Replacements, we don't serialize the filename because in the context of the migrator, we know every replacement is for the same file and we serialize that name only once. If we wanted to keep this functionality in the general-purpose implementation in tooling, we'd need to provide some mechanism for turning that feature on or off. Granted, I haven't done any performance measurement of (de)serializing large YAML files with the filename present and not present to see if it's even worth it.

  I can remove mention of 'headers' in names and comments as you like.

  'migmerge' is a strawman name until someone can think up something better.


================
Comment at: cpp11-migrate/Core/ApplyChangeDescriptions.cpp:115
@@ +114,3 @@
+    std::vector<tooling::Range> Conflicts;
+    deduplicate(I->getValue(), Conflicts);
+
----------------
Guillaume Papin wrote:
> I'm surprised, `deduplicate()` is in `tooling::` no? I don't see any `using tooling` or similar.
That's argument-dependent name lookup working for you:)

================
Comment at: cpp11-migrate/tool/CMakeLists.txt:46-48
@@ +45,5 @@
+
+set(LLVM_OPTIONAL_SOURCES
+  Cpp11Migrate.cpp
+  )
+
----------------
Guillaume Papin wrote:
> 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.
The point is Cpp11Migrate.cpp must be optional for building migmerge and MergeMain.cpp must be optional when building the migrator. Otherwise CMake will complain about finding source files that are not explicitly listed for building. This is necessary because we're building two executables in the same CMakeLists.txt.

================
Comment at: cpp11-migrate/tool/MergeMain.cpp:13
@@ +12,3 @@
+Directory(cl::Positional, cl::Required,
+          cl::desc("<Search Root Directory>"));
+
----------------
Guillaume Papin wrote:
> I think the angles aren't needed.
They're not **needed** but they're common practice for command-line args:

  mytool <arg>

implies <arg> is necessary.

================
Comment at: test/migmerge/conflict/file3.yaml:2
@@ +1,3 @@
+---
+TransformID:     "Blah"
+Replacements:
----------------
Manuel Klimek wrote:
> Is the TransformID actually used for anything?
Not currently. The migrator will actually put something useful here to indicate which transform these replacements were generated by. It's probably not necessary any longer since the migrator applies only one transform at a time.


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