@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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
