[clang-apply-replacements] Convert tooling::Replacements to tooling::AtomicChange for conflict resolving of changes, code cleanup, and code formatting.

[jdemeule]

Summary

By converting Replacements by AtomicChange, clang-apply-replacements is able like clang-tidy to automatically cleanup and format changes.
This should permits to close this ticket: https://bugs.llvm.org/show_bug.cgi?id=35051 and attempt to follow hints from https://reviews.llvm.org/D43500 comments.

[ioeric]

Thanks for the cleanup! This looks really nice!

clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h

45 ↗

(On Diff #135903)

I would expect this to be a map from files to a group of AtomicChanges (e.g. std::vector<AtomicChange>). In general, a single AtomicChange contains changes around a single code location which are likely to conflict with each other, and either all changes or no change is applied. A file usually corresponds to a set of atomic changes.

Intuitively, I think clang-apply-replacements should simple gather a set of atomic changes for each file and let applyAtomicChanges handle the conflicts, but its current behavior is to skip conflicting replacements and keep applying other replacements. This is not ideal, but I guess I understand where this came from, and we might not be able to fix this in this patch since most tools produce replacements instead of AtomicChange.

I would still suggest make this a map something like llvm::DenseMap<const clang::FileEntry *, std::vector<tooling::AtomicChange>>. To preserve the current behavior (i.e. skip conflicts), when you group all replacements for a single file, you would still put all replacements into a single AtomicChange, but when you actually put the change into the map, you put it as a vector of a single change.

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

207 ↗

(On Diff #135903)

I don't think we need to do the deduplication here anymore. AtomicChange handles duplicates for you. I think all you need to do here is to group replacements by files and later convert replacements to atomic changes.

248 ↗

(On Diff #135903)

(Related to my comment on FileToChangeMap)

We should add FIXME here explaining why we are putting all replacements for a single file into one AtomicChange (e.g. we want to be able to detect and report each individual conflict) instead of a set of changes.

259 ↗

(On Diff #135903)

(Also related to my comment on FileToChangeMap)

This would be something like

Changes.insert(std::make_pair(Entry, {Change}));

279 ↗

(On Diff #135903)

You should handle the error in llvm::Expected. You could convert it to string and add to the error message with llvm::toString(NewCode.takeError()). It would be nice if we could have a test case for such cases.

[jdemeule]

Thanks for your feedback, they are very precious to me!

clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h

45 ↗

(On Diff #135903)

I got your point. I will update the patch this way.

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

207 ↗

(On Diff #135903)

I think I wrongly use AtomicChange somewhere because it doesn't deduplicate same replacement automatically.
For exemple, in the test suite, basic test defines 2 time the same replacement (adding 'override ' at offset 148) and I do not manage to avoid AtomicChange to add 'override override '. This is why I have kept the deduplicate step.

279 ↗

(On Diff #135903)

I will use llvm::Expected as you suggest.
Do you have some ideas to made a test failed at this level?

[ioeric]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

207 ↗

(On Diff #135903)

AtomicChange does deduplicate identical replacements but not insertions, because it wouldn't know whether users really want the insertions to be deduplicated or not (e.g. imagine a tool wants to insert two ) at the same location). So it doesn't support that test case intentionally. In general, users (i.e. tools generating changes) are responsible for ensuring changes are deduplicated/applied in the expected way by using the correct interface (e.g. replace, insert etc). I think it would probably make more sense to change the test to deduplicate identical non-insertion replacements. It would also make sense to add another test case where identical insertions are both applied.

For more semantics of conflicting/duplicated replacements, see https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Core/Replacement.h#L217

279 ↗

(On Diff #135903)

I will use llvm::Expected as you suggest.

I think NewCode is already llvm::Expected<std::string>. You would only need to explicitly handle the error.

Do you have some ideas to made a test failed at this level?

That's a good question. I think we would really need unit tests for the ApplyReplacements library in order to get better coverage. But it's probably out of the scope of this patch, so I'd also be fine without the test. Up to you :)

[jdemeule]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

207 ↗

(On Diff #135903)

That's make sense indeed. My confusion comes from I do not want to break existing tests. I will update them the way you describe.

[jdemeule]

Update patch after reviewer comments.

[ioeric]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

188 ↗

(On Diff #136944)

Add a brief comment describe what this function does, specifically about TUs and TUDs.

190 ↗

(On Diff #136944)

nit: SM can be const. And put SM (input arg) before FileChanges (output arg).

213–221 ↗

(On Diff #136944)

This code block is the same as the one in the above loop. Consider pulling it into a lambda.

276–279 ↗

(On Diff #136944)

AtomicChange::replace also handles insertions, so I think there is no need for the distinction here.

(See my comment in where ReplacementsToAtomicChanges is used; I think the class might not be needed.)

290 ↗

(On Diff #136944)

I might be missing something, but why do we need to pull replacements from the result change into a set of changes?

317 ↗

(On Diff #136944)

By default, this inserts R after the existing insertion in the same location (if there is any). But it's impossible for apply-all-replacements to know in which order they should be applied, so I would suggest treating this as conflict. Sorry that I might have confused you by asking to add a test case where two identical insertions are both applied (because they are order *independent*), but AtomicChange::replace also supports that and would report error when two insertions are order *dependent*.

AtomicChange::replace and AtomicChange::insert have almost the same semantic except that you could specify the expected order of insertions on the same location with insert, but in our case, we wouldn't know which order is desired.

353 ↗

(On Diff #136944)

Sorry that I didn't make myself clear... what you had in the previous revision was correct (for the current use case of apply-all-replacements) i.e. store all replacements in one AtomicChange, which allows you to detect conflicts on the fly. So the code here can be simplified as:

...
Entry = ...;
AtomicChange FileChange;
for (const auto &R : FileAndReplacements.second) {
  auto Err = FileChange.replace( <args from R> );
  if (Err)
    reportConflict(Entry, std::move(Err));  // reportConflict as we go
}
FileChanges.emplace(Entry, {FileChange});
...

I think with this set up, you wouldn't need ReplacementsToAtomicChanges, ConflictError and reportConflicts.

test/clang-apply-replacements/Inputs/basic/file2.yaml

1 ↗

(On Diff #136944)

Could you please add two test cases for insertions: 1) identical insertions (e.g. "AB" and "AB") at the same location are applied sucessfully and 2) order-dependent insertions (e.g. "AB" and "BA") are detected.

(It might make sense to do this in a different file)

test/clang-apply-replacements/Inputs/conflict/expected.txt

8 ↗

(On Diff #136944)

How could one replacement (Remove 12:3-12:14) conflict with itself?

[jdemeule]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

213–221 ↗

(On Diff #136944)

Yes, for sure. I was not confident about this part.

290 ↗

(On Diff #136944)

I interpret you suggestion of set of AtomicChange a little bit too much.

353 ↗

(On Diff #136944)

I think I have over-interpreting your previous comment :-)
However, I am still confused about error reporting.
Currently, clang-apply-replacements reports conflicting replacement per file and per conflict.
For example:

There are conflicting changes to XXX:
The following changes conflict:
  Replace 8:8-8:33 with "AAA"
  Replace 8:8-8:33 with "BBB"
  Remove 8:10-8:15
  Insert at 8:14 CCC

With this patch, conflict are reported by pair (first replacement/conflicted one) and I am able to print the corresponding file only once (thanks to the defered reporting).

There are conflicting changes to XXX:
The following changes conflict:
  Replace 8:8-8:33 with "AAA"
  Replace 8:8-8:33 with "BBB"
The following changes conflict:
  Replace 8:8-8:33 with "AAA"
  Remove 8:10-8:15
The following changes conflict:
  Replace 8:8-8:33 with "AAA"
  Insert at 8:14 CCC

I prefer the way you suggest to report conflict but we will loose or print conflicting file at each conflict detected.
I even more prefer to use llvm::toString(std::move(Err)) but this will fully change the reporting and will also be reported by pair.

The new replacement overlaps with an existing replacement.
New replacement: XXX: 106:+26:"AAA"
Existing replacement: XXX: 106:+26:"BBB"
The new replacement overlaps with an existing replacement.
New replacement: XXX: 106:+26:"AAA"
Existing replacement: XXX: 112:+0:"CCC"
The new replacement overlaps with an existing replacement.
New replacement: XXX: 106:+26:"AAA"
Existing replacement: XXX: 108:+12:""

test/clang-apply-replacements/Inputs/basic/file2.yaml

1 ↗

(On Diff #136944)

I will.

test/clang-apply-replacements/Inputs/conflict/expected.txt

8 ↗

(On Diff #136944)

It is not the case, they are reported by pair now.

[jdemeule]

Update after review.
Add tests to check identical insertions and order dependent insertions.

[ioeric]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

353 ↗

(On Diff #136944)

I am inclined to changing the current behavior and reporting by pairs because

1. this would simplify the code a lot.because we could basically reuse the conflict detection from replacement library.
2. IMO, pairs are easier to read than grouping multiple conflicts - users would still need to interpret the conflicts and figure out how all replacements conflict in a group, while reporting as pairs already gives you this information.
3. in practice, it's uncommon to see more than two conflicting replacements at the same code location.

I would vote for the last approach (with llvm::toString(std::move(Err))) which is easy to implement and doesn't really regress the UI that much. WDYT?

[jdemeule]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

353 ↗

(On Diff #136944)

I think if we can use llvm::toString(std::move(Err)) for this patch, it will simplify the code and reuse already existing error message. Even if I found file+offset conflict location less readable than file+line+column.

I have made some prototype to "enhance" error reporting but I think they should be put in dedicated patches and need further discutions.
During my "research" I found we can use:

• Conflict marker format.

/src/basic.h
@@ 12:23-12:23 @@
<<<<<<< Existing replacement
  virtual void func() noexcept {}
=======
  virtual void func() override {}
>>>>>>> New replacement

• A unified diff like.

/src/basic.h
@@ 12:23-12:23 @@
-   virtual void func() {}
+   virtual void func() noexcept {}
+   virtual void func() override {}

• A clang like diagnostic message.

/src/basic.h
@@ 12:23-12:23 @@
  virtual void func() {}
                      ^
                      noexcept 
  virtual void func() {}
                      ^
                      override

[ioeric]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

353 ↗

(On Diff #136944)

The conflict marker format looks promising!

You are right regarding offset vs line+column location. I agree offset is less readable. I think we could probably build offset -> line+column translation into ReplacementError if necessary. For example, we could add a pretty-print method/helper that takes in source code and outputs readable error messages with offset translated to line+columns according to the source code, which is basically what the current replaceConflict function does. Ideally, that would also handle/consume the underlying errors so that we could switch to use that instead of llvm::toString.

[jdemeule]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

353 ↗

(On Diff #136944)

Do you prefer to add the transformation offset -> line+column in this patch or in a future one (with an option to switch between line report and conflict marker for example)?

[ioeric]

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

353 ↗

(On Diff #136944)

I think it would be easier to have those in a different patch. For this patch, reporting conflict pairs with either offsets or line+column (by using the transformation that is already there) would be fine (with a FIXME maybe).

(These are all great improvements! Thanks for doing this!)

[jdemeule]

Update after review.
Change error reporting to use ReplacementError message.

[ioeric]

This looks great! Just a few nits left.

clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h

75 ↗

(On Diff #139082)

If conflicts occur, no Replacements are applied. This doesn't seem to be accurate; non-conflicting replacements are still added.

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

173 ↗

(On Diff #139082)

nit: I'd probably use auto GroupedReplacements = ...;.

179 ↗

(On Diff #139082)

I think we are only skipping conflicts; non-conflicting replacements are still added even if the file contains other conflicts?

This doesn't seem to have caused any problem because the current caller simply drops all changes if conflict is detected in any file, but we should still make it clear what the behavior of mergeAndDeduplicate is (in the API doc) e.g. what would FileChanges contain if there is conflict in some but not all files?

218 ↗

(On Diff #139082)

nit: redundant empty line.

227 ↗

(On Diff #139082)

nit: consider inline this to save one variable.

[jdemeule]

clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h

75 ↗

(On Diff #139082)

You are perfectly right the description does not match the behavior.

I think I will update this part to "Deduplicate, check for conflicts, and convert all Replacements stored in TUs to a single AtomicChange per file. Conflicting replacement are skipped. However, the order of converting Replacements extracted from TUs and TUDs to AtomicChange is undefined."

And amend FileChanges with "Only non conflicting replacements are kept into FileChanges."

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

179 ↗

(On Diff #139082)

I will update this comment to:
"To keep current clang-apply-replacement behavior, report conflicting replacements on corresponding file, all replacements are stored into 1 big AtomicChange."

[jdemeule]

Update after review (cleanup and improve comments).

[ioeric]

lgtm

clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h

75 ↗

(On Diff #139082)

a single AtomicChange per file

As FileChanges are a FileToChangesMap, it's already clear that it's a set of changes per file. We don't want to say "single AtomicChange" here because this is just an implementation details.

clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

179 ↗

(On Diff #139082)

I would drop the "To keep the current behavior" part because it's only relevant in code review. Future readers wouldn't know what the "current" behavior is.

[jdemeule]

Thanks a lot!
Can I ask you if I need to do something as I do not have commit access?

[jdemeule]

Can I help in some way to close this PR?

[malcolm.parsons]

In D43764#1058120, @jdemeule wrote:

Can I help in some way to close this PR?

I applied the patch and ran the tests.

There was one unexpected failure:

[100%] Running the Clang extra tools' regression tests
FAIL: Clang Tools :: clang-apply-replacements/conflict.cpp (10 of 896)
******************** TEST 'Clang Tools :: clang-apply-replacements/conflict.cpp' FAILED ********************
Script:
--
mkdir -p /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict
sed "s#\$(path)#/mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict#" /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/file1.yaml > /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict/file1.yaml
sed "s#\$(path)#/mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict#" /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/file2.yaml > /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict/file2.yaml
sed "s#\$(path)#/mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict#" /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/file3.yaml > /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict/file3.yaml
sed "s#\$(path)#/mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict#" /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/expected.txt > /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict/expected.txt
not clang-apply-replacements /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict > /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict/output.txt 2>&1
diff -b /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict/output.txt /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict/expected.txt
ls -1 /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict | FileCheck /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/conflict.cpp --check-prefix=YAML
not clang-apply-replacements /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict -remove-change-desc-files > /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict/output.txt 2>&1
ls -1 /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict | FileCheck /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/conflict.cpp --check-prefix=NO_YAML
--
Exit Code: 1

Command Output (stdout):
--
2,4d1
< New replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 169:+0:"(int*)"
< Existing replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 160:+12:""
< The new replacement overlaps with an existing replacement.
11a9,11
> Existing replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 160:+12:""
> The new replacement overlaps with an existing replacement.
> New replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 169:+0:"(int*)"

--

********************
Testing Time: 21.09s
********************
Failing Tests (1):
    Clang Tools :: clang-apply-replacements/conflict.cpp

  Expected Passes    : 893
  Unsupported Tests  : 2
  Unexpected Failures: 1

[jdemeule]

Thank you for applying the patch.

I suspect some undefined order when applying replacements as directly dependant in which order OS reads file1.yaml, file2.yamland file3.yaml.
As I am not able to reproduce it on my machine, can you share with me the content of output.txt?

[malcolm.parsons]

In D43764#1061042, @jdemeule wrote:

I suspect some undefined order when applying replacements as directly dependent on which order OS reads file1.yaml, file2.yamland file3.yaml.

$ ls -f /mparsons/llvm/llvm-build/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/conflict/
file1.yaml .. output.txt . file3.yaml file2.yaml expected.txt

As I am not able to reproduce it on my machine, can you share with me the content of output.txt?

The new replacement overlaps with an existing replacement.
New replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 169:+0:"(int*)"
Existing replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 160:+12:""
The new replacement overlaps with an existing replacement.
New replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 106:+26:"int & elem : ints"
Existing replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 106:+26:"auto & i : ints"
The new replacement overlaps with an existing replacement.
New replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 140:+7:"elem"
Existing replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 140:+7:"i"
The new replacement overlaps with an existing replacement.
New replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 169:+1:"nullptr"
Existing replacement: /mparsons/llvm/llvm/tools/clang/tools/extra/test/clang-apply-replacements/Inputs/conflict/common.h: 160:+12:""

[jdemeule]

Ok, I understand the problem.
Previously, during the deduplication step, replacements per file where sorted and it is not the case anymore.
That means now, clang-apply-replacement is highly dependant of reading file order during error reporting.
I mainly see 2 options (I can miss others), rewrite the test (e.g merging yaml file together) or sort the replacements per file to fix the order of application.

Can I ask you what solution you prefer?

[malcolm.parsons]

In D43764#1062748, @jdemeule wrote:

Ok, I understand the problem.
Previously, during the deduplication step, replacements per file where sorted and it is not the case anymore.
That means now, clang-apply-replacement is highly dependant of reading file order during error reporting.
I mainly see 2 options (I can miss others), rewrite the test (e.g merging yaml file together) or sort the replacements per file to fix the order of application.

Can I ask you what solution you prefer?

I'd prefer sorting, so that results will be consistent when the tool is used on real data.

[jdemeule]

Sort replacements before converting them to AtomicChange.

[lebedev.ri]

Third commit introducing linking errors.
Did no buildbot catch those?

[malcolm.parsons]

In D43764#1065345, @lebedev.ri wrote:

Third commit introducing linking errors.
Did no buildbot catch those?

2 buildbots sent me an email, but they were unrelated failures.

clang-tools-extra/trunk/clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h

 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_APPLYREPLACEMENTS_H
 #define LLVM_CLANG_APPLYREPLACEMENTS_H
 
 #include "clang/Tooling/Core/Diagnostic.h"
 #include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include <string>
 #include <system_error>
 #include <vector>
 
 namespace clang {
 
 class DiagnosticsEngine;
 class Rewriter;
 
-namespace format {
-struct FormatStyle;
-} // end namespace format
-
 namespace replace {
 
-/// \brief Collection of source ranges.
-typedef std::vector<clang::tooling::Range> RangeVector;
-
 /// \brief Collection of TranslationUnitReplacements.
 typedef std::vector<clang::tooling::TranslationUnitReplacements> TUReplacements;
 
 /// \brief Collection of TranslationUnitReplacement files.
 typedef std::vector<std::string> TUReplacementFiles;
 
 /// \brief Collection of TranslationUniDiagnostics.
 typedef std::vector<clang::tooling::TranslationUnitDiagnostics> TUDiagnostics;
 
-/// \brief Map mapping file name to Replacements targeting that file.
+/// \brief Map mapping file name to a set of AtomicChange targeting that file.
 typedef llvm::DenseMap<const clang::FileEntry *,
-                       std::vector<clang::tooling::Replacement>>
-    FileToReplacementsMap;
+                       std::vector<tooling::AtomicChange>>
+    FileToChangesMap;
 
 /// \brief Recursively descends through a directory structure rooted at \p
 /// Directory and attempts to deserialize *.yaml files as
 /// TranslationUnitReplacements. All docs that successfully deserialize are
 /// added to \p TUs.
 ///
 /// Directories starting with '.' are ignored during traversal.
 ///

 std::error_code collectReplacementsFromDirectory(
     const llvm::StringRef Directory, TUReplacements &TUs,
     TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics);
 
 std::error_code collectReplacementsFromDirectory(
     const llvm::StringRef Directory, TUDiagnostics &TUs,
     TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics);
 
-/// \brief Deduplicate, check for conflicts, and apply all Replacements stored
-/// in \c TUs. If conflicts occur, no Replacements are applied.
+/// \brief Deduplicate, check for conflicts, and extract all Replacements stored
+/// in \c TUs. Conflicting replacements are skipped.
 ///
-/// \post For all (key,value) in GroupedReplacements, value[i].getOffset() <=
+/// \post For all (key,value) in FileChanges, value[i].getOffset() <=
 /// value[i+1].getOffset().
 ///
 /// \param[in] TUs Collection of TranslationUnitReplacements or
-/// TranslationUnitDiagnostics to merge,
-/// deduplicate, and test for conflicts.
-/// \param[out] GroupedReplacements Container grouping all Replacements by the
-/// file they target.
+/// TranslationUnitDiagnostics to merge, deduplicate, and test for conflicts.
+/// \param[out] FileChanges Container grouping all changes by the
+/// file they target. Only non conflicting replacements are kept into
+/// FileChanges.
 /// \param[in] SM SourceManager required for conflict reporting.
 ///
 /// \returns \parblock
-///          \li true If all changes were applied successfully.
+///          \li true If all changes were converted successfully.
 ///          \li false If there were conflicts.
-bool mergeAndDeduplicate(const TUReplacements &TUs,
-                         FileToReplacementsMap &GroupedReplacements,
-                         clang::SourceManager &SM);
-
-bool mergeAndDeduplicate(const TUDiagnostics &TUs,
-                         FileToReplacementsMap &GroupedReplacements,
+bool mergeAndDeduplicate(const TUReplacements &TUs, const TUDiagnostics &TUDs,
+                         FileToChangesMap &FileChanges,
                          clang::SourceManager &SM);
 
-// FIXME: Remove this function after changing clang-apply-replacements to use
-// Replacements class.
-bool applyAllReplacements(const std::vector<tooling::Replacement> &Replaces,
-                          Rewriter &Rewrite);
-
-/// \brief Apply all replacements in \c GroupedReplacements.
-///
-/// \param[in] GroupedReplacements Deduplicated and conflict free Replacements
-/// to apply.
-/// \param[out] Rewrites The results of applying replacements will be applied
-/// to this Rewriter.
+/// \brief Apply \c AtomicChange on File and rewrite it.
 ///
-/// \returns \parblock
-///          \li true If all changes were applied successfully.
-///          \li false If a replacement failed to apply.
-bool applyReplacements(const FileToReplacementsMap &GroupedReplacements,
-                       clang::Rewriter &Rewrites);
-
-/// \brief Given a collection of Replacements for a single file, produces a list
-/// of source ranges that enclose those Replacements.
-///
-/// \pre Replacements[i].getOffset() <= Replacements[i+1].getOffset().
-///
-/// \param[in] Replacements Replacements from a single file.
-///
-/// \returns Collection of source ranges that enclose all given Replacements.
-/// One range is created for each replacement.
-RangeVector calculateChangedRanges(
-    const std::vector<clang::tooling::Replacement> &Replacements);
-
-/// \brief Write the contents of \c FileContents to disk. Keys of the map are
-/// filenames and values are the new contents for those files.
+/// \param[in] File Path of the file where to apply AtomicChange.
+/// \param[in] Changes to apply.
+/// \param[in] Spec For code cleanup and formatting.
+/// \param[in] Diagnostics DiagnosticsEngine used for error output.
 ///
-/// \param[in] Rewrites Rewriter containing written files to write to disk.
-bool writeFiles(const clang::Rewriter &Rewrites);
+/// \returns The changed code if all changes are applied successfully;
+/// otherwise, an llvm::Error carrying llvm::StringError or an error_code.
+llvm::Expected<std::string>
+applyChanges(StringRef File, const std::vector<tooling::AtomicChange> &Changes,
+             const tooling::ApplyChangesSpec &Spec,
+             DiagnosticsEngine &Diagnostics);
 
 /// \brief Delete the replacement files.
 ///
 /// \param[in] Files Replacement files to delete.
 /// \param[in] Diagnostics DiagnosticsEngine used for error output.
 ///
 /// \returns \parblock
 ///          \li true If all files have been deleted successfully.

clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

for (recursive_directory_iterator I(Directory, ErrorCode), E;
 
     // Only keep files that properly parse.
     TUs.push_back(TU);
   }
 
   return ErrorCode;
 }
 
-std::error_code
-collectReplacementsFromDirectory(const llvm::StringRef Directory,
-                                 TUDiagnostics &TUs, TUReplacementFiles &TUFiles,
-                                 clang::DiagnosticsEngine &Diagnostics) {
+std::error_code collectReplacementsFromDirectory(
+    const llvm::StringRef Directory, TUDiagnostics &TUs,
+    TUReplacementFiles &TUFiles, clang::DiagnosticsEngine &Diagnostics) {
   using namespace llvm::sys::fs;
   using namespace llvm::sys::path;
 
   std::error_code ErrorCode;
 
   for (recursive_directory_iterator I(Directory, ErrorCode), E;
        I != E && !ErrorCode; I.increment(ErrorCode)) {
     if (filename(I->path())[0] == '.') {
for (recursive_directory_iterator I(Directory, ErrorCode), E;
 
     // Only keep files that properly parse.
     TUs.push_back(TU);
   }
 
   return ErrorCode;
 }
 
-/// \brief Dumps information for a sequence of conflicting Replacements.
+/// \brief Extract replacements from collected TranslationUnitReplacements and
+/// TranslationUnitDiagnostics and group them per file.
 ///
-/// \param[in] File FileEntry for the file the conflicting Replacements are
-/// for.
-/// \param[in] ConflictingReplacements List of conflicting Replacements.
-/// \param[in] SM SourceManager used for reporting.
-static void reportConflict(
-    const FileEntry *File,
-    const llvm::ArrayRef<clang::tooling::Replacement> ConflictingReplacements,
-    SourceManager &SM) {
-  FileID FID = SM.translateFile(File);
-  if (FID.isInvalid())
-    FID = SM.createFileID(File, SourceLocation(), SrcMgr::C_User);
-
-  // FIXME: Output something a little more user-friendly (e.g. unified diff?)
-  errs() << "The following changes conflict:\n";
-  for (const tooling::Replacement &R : ConflictingReplacements) {
-    if (R.getLength() == 0) {
-      errs() << "  Insert at " << SM.getLineNumber(FID, R.getOffset()) << ":"
-             << SM.getColumnNumber(FID, R.getOffset()) << " "
-             << R.getReplacementText() << "\n";
-    } else {
-      if (R.getReplacementText().empty())
-        errs() << "  Remove ";
-      else
-        errs() << "  Replace ";
-
-      errs() << SM.getLineNumber(FID, R.getOffset()) << ":"
-             << SM.getColumnNumber(FID, R.getOffset()) << "-"
-             << SM.getLineNumber(FID, R.getOffset() + R.getLength() - 1) << ":"
-             << SM.getColumnNumber(FID, R.getOffset() + R.getLength() - 1);
-
-      if (R.getReplacementText().empty())
-        errs() << "\n";
-      else
-        errs() << " with \"" << R.getReplacementText() << "\"\n";
-    }
-  }
-}
-
-// FIXME: Remove this function after changing clang-apply-replacements to use
-// Replacements class.
-bool applyAllReplacements(const std::vector<tooling::Replacement> &Replaces,
-                          Rewriter &Rewrite) {
-  bool Result = true;
-  for (auto I = Replaces.begin(), E = Replaces.end(); I != E; ++I) {
-    if (I->isApplicable()) {
-      Result = I->apply(Rewrite) && Result;
-    } else {
-      Result = false;
-    }
-  }
-  return Result;
-}
-
-// FIXME: moved from libToolingCore. remove this when std::vector<Replacement>
-// is replaced with tooling::Replacements class.
-static void deduplicate(std::vector<tooling::Replacement> &Replaces,
-                        std::vector<tooling::Range> &Conflicts) {
-  if (Replaces.empty())
-    return;
-
-  auto LessNoPath = [](const tooling::Replacement &LHS,
-                       const tooling::Replacement &RHS) {
-    if (LHS.getOffset() != RHS.getOffset())
-      return LHS.getOffset() < RHS.getOffset();
-    if (LHS.getLength() != RHS.getLength())
-      return LHS.getLength() < RHS.getLength();
-    return LHS.getReplacementText() < RHS.getReplacementText();
-  };
-
-  auto EqualNoPath = [](const tooling::Replacement &LHS,
-                        const tooling::Replacement &RHS) {
-    return LHS.getOffset() == RHS.getOffset() &&
-           LHS.getLength() == RHS.getLength() &&
-           LHS.getReplacementText() == RHS.getReplacementText();
-  };
-
-  // Deduplicate. We don't want to deduplicate based on the path as we assume
-  // that all replacements refer to the same file (or are symlinks).
-  std::sort(Replaces.begin(), Replaces.end(), LessNoPath);
-  Replaces.erase(std::unique(Replaces.begin(), Replaces.end(), EqualNoPath),
-                 Replaces.end());
-
-  // Detect conflicts
-  tooling::Range ConflictRange(Replaces.front().getOffset(),
-                               Replaces.front().getLength());
-  unsigned ConflictStart = 0;
-  unsigned ConflictLength = 1;
-  for (unsigned i = 1; i < Replaces.size(); ++i) {
-    tooling::Range Current(Replaces[i].getOffset(), Replaces[i].getLength());
-    if (ConflictRange.overlapsWith(Current)) {
-      // Extend conflicted range
-      ConflictRange =
-          tooling::Range(ConflictRange.getOffset(),
-                         std::max(ConflictRange.getLength(),
-                                  Current.getOffset() + Current.getLength() -
-                                      ConflictRange.getOffset()));
-      ++ConflictLength;
-    } else {
-      if (ConflictLength > 1)
-        Conflicts.push_back(tooling::Range(ConflictStart, ConflictLength));
-      ConflictRange = Current;
-      ConflictStart = i;
-      ConflictLength = 1;
-    }
-  }
-
-  if (ConflictLength > 1)
-    Conflicts.push_back(tooling::Range(ConflictStart, ConflictLength));
-}
-
-/// \brief Deduplicates and tests for conflicts among the replacements for each
-/// file in \c Replacements. Any conflicts found are reported.
-///
-/// \post Replacements[i].getOffset() <= Replacements[i+1].getOffset().
-///
-/// \param[in,out] Replacements Container of all replacements grouped by file
-/// to be deduplicated and checked for conflicts.
-/// \param[in] SM SourceManager required for conflict reporting.
-///
-/// \returns \parblock
-///          \li true if conflicts were detected
-///          \li false if no conflicts were detected
-static bool deduplicateAndDetectConflicts(FileToReplacementsMap &Replacements,
-                                          SourceManager &SM) {
-  bool conflictsFound = false;
-
-  for (auto &FileAndReplacements : Replacements) {
-    const FileEntry *Entry = FileAndReplacements.first;
-    auto &Replacements = FileAndReplacements.second;
-    assert(Entry != nullptr && "No file entry!");
-
-    std::vector<tooling::Range> Conflicts;
-    deduplicate(FileAndReplacements.second, Conflicts);
-
-    if (Conflicts.empty())
-      continue;
-
-    conflictsFound = true;
-
-    errs() << "There are conflicting changes to " << Entry->getName() << ":\n";
-
-    for (const tooling::Range &Conflict : Conflicts) {
-      auto ConflictingReplacements = llvm::makeArrayRef(
-          &Replacements[Conflict.getOffset()], Conflict.getLength());
-      reportConflict(Entry, ConflictingReplacements, SM);
-    }
-  }
-
-  return conflictsFound;
-}
-
-bool mergeAndDeduplicate(const TUReplacements &TUs,
-                         FileToReplacementsMap &GroupedReplacements,
-                         clang::SourceManager &SM) {
-
-  // Group all replacements by target file.
+/// \param[in] TUs Collection of all found and deserialized
+/// TranslationUnitReplacements.
+/// \param[in] TUDs Collection of all found and deserialized
+/// TranslationUnitDiagnostics.
+/// \param[in] SM Used to deduplicate paths.
+///
+/// \returns A map mapping FileEntry to a set of Replacement targeting that
+/// file.
+static llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
+groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
+                  const clang::SourceManager &SM) {
   std::set<StringRef> Warned;
-  for (const auto &TU : TUs) {
-    for (const tooling::Replacement &R : TU.Replacements) {
-      // Use the file manager to deduplicate paths. FileEntries are
-      // automatically canonicalized.
-      if (const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath())) {
-        GroupedReplacements[Entry].push_back(R);
-      } else if (Warned.insert(R.getFilePath()).second) {
-          errs() << "Described file '" << R.getFilePath()
-                 << "' doesn't exist. Ignoring...\n";
-      }
-    }
-  }
-
-  // Ask clang to deduplicate and report conflicts.
-  return !deduplicateAndDetectConflicts(GroupedReplacements, SM);
-}
+  llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
+      GroupedReplacements;
 
-bool mergeAndDeduplicate(const TUDiagnostics &TUs,
-                         FileToReplacementsMap &GroupedReplacements,
-                         clang::SourceManager &SM) {
-
-  // Group all replacements by target file.
-  std::set<StringRef> Warned;
-  for (const auto &TU : TUs) {
-    for (const auto &D : TU.Diagnostics) {
-      for (const auto &Fix : D.Fix) {
-        for (const tooling::Replacement &R : Fix.second) {
-          // Use the file manager to deduplicate paths. FileEntries are
-          // automatically canonicalized.
-          if (const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath())) {
-            GroupedReplacements[Entry].push_back(R);
-          } else if (Warned.insert(R.getFilePath()).second) {
-              errs() << "Described file '" << R.getFilePath()
-                     << "' doesn't exist. Ignoring...\n";
-          }
-        }
-      }
-    }
-  }
-
-  // Ask clang to deduplicate and report conflicts.
-  return !deduplicateAndDetectConflicts(GroupedReplacements, SM);
-}
-
-bool applyReplacements(const FileToReplacementsMap &GroupedReplacements,
-                       clang::Rewriter &Rewrites) {
-
-  // Apply all changes
-  //
-  // FIXME: No longer certain GroupedReplacements is really the best kind of
-  // data structure for applying replacements. Rewriter certainly doesn't care.
-  // However, until we nail down the design of ReplacementGroups, might as well
-  // leave this as is.
-  for (const auto &FileAndReplacements : GroupedReplacements) {
-    if (!applyAllReplacements(FileAndReplacements.second, Rewrites))
-      return false;
-  }
+  auto AddToGroup = [&](const tooling::Replacement &R) {
+  };
 
-  return true;
-}
+  for (const auto &TU : TUs)
+    for (const tooling::Replacement &R : TU.Replacements)
+      AddToGroup(R);
 
-RangeVector calculateChangedRanges(
-    const std::vector<clang::tooling::Replacement> &Replaces) {
-  RangeVector ChangedRanges;
+  for (const auto &TU : TUDs)
+    for (const auto &D : TU.Diagnostics)
+      for (const auto &Fix : D.Fix)
+        for (const tooling::Replacement &R : Fix.second)
+          AddToGroup(R);
 
-  // Generate the new ranges from the replacements.
-  int Shift = 0;
-  for (const tooling::Replacement &R : Replaces) {
-    unsigned Offset = R.getOffset() + Shift;
-    unsigned Length = R.getReplacementText().size();
-    Shift += Length - R.getLength();
-    ChangedRanges.push_back(tooling::Range(Offset, Length));
+  // Sort replacements per file to keep consistent behavior when
+  // clang-apply-replacements run on differents machine.
+  for (auto &FileAndReplacements : GroupedReplacements) {
+    llvm::sort(FileAndReplacements.second.begin(),
+               FileAndReplacements.second.end());
   }
 
-  return ChangedRanges;
+  return GroupedReplacements;
 }
 
-bool writeFiles(const clang::Rewriter &Rewrites) {
-
-  for (auto BufferI = Rewrites.buffer_begin(), BufferE = Rewrites.buffer_end();
-       BufferI != BufferE; ++BufferI) {
-    StringRef FileName =
-        Rewrites.getSourceMgr().getFileEntryForID(BufferI->first)->getName();
-
-    std::error_code EC;
-    llvm::raw_fd_ostream FileStream(FileName, EC, llvm::sys::fs::F_Text);
-    if (EC) {
-      errs() << "Warning: Could not write to " << EC.message() << "\n";
-      continue;
-    }
-    BufferI->second.write(FileStream);
-  }
+bool mergeAndDeduplicate(const TUReplacements &TUs, const TUDiagnostics &TUDs,
+                         FileToChangesMap &FileChanges,
+                         clang::SourceManager &SM) {
+  auto GroupedReplacements = groupReplacements(TUs, TUDs, SM);
+  bool ConflictDetected = false;
 
-  return true;
+  // To report conflicting replacements on corresponding file, all replacements
+  // are stored into 1 big AtomicChange.
+  for (const auto &FileAndReplacements : GroupedReplacements) {
+    const FileEntry *Entry = FileAndReplacements.first;
+    const SourceLocation BeginLoc =
+        SM.getLocForStartOfFile(SM.getOrCreateFileID(Entry, SrcMgr::C_User));
+    tooling::AtomicChange FileChange(Entry->getName(), Entry->getName());
+    for (const auto &R : FileAndReplacements.second) {
+      llvm::Error Err =
+          FileChange.replace(SM, BeginLoc.getLocWithOffset(R.getOffset()),
+                             R.getLength(), R.getReplacementText());
+      if (Err) {
+        // FIXME: This will report conflicts by pair using a file+offset format
+        // which is not so much human readable.
+        // A first improvement could be to translate offset to line+col. For
+        // this and without loosing error message some modifications arround
+        // `tooling::ReplacementError` are need (access to
+        // `getReplacementErrString`).
+        // A better strategy could be to add a pretty printer methods for
+        // conflict reporting. Methods that could be parameterized to report a
+        // conflict in different format, file+offset, file+line+col, or even
+        // more human readable using VCS conflict markers.
+        // For now, printing directly the error reported by `AtomicChange` is
+        // the easiest solution.
+        errs() << llvm::toString(std::move(Err)) << "\n";
+        ConflictDetected = true;
+      }
+    }
+    FileChanges.try_emplace(Entry,
+                            std::vector<tooling::AtomicChange>{FileChange});
+  }
+
+  return !ConflictDetected;
+}
+
+llvm::Expected<std::string>
+applyChanges(StringRef File, const std::vector<tooling::AtomicChange> &Changes,
+             const tooling::ApplyChangesSpec &Spec,
+             DiagnosticsEngine &Diagnostics) {
+  FileManager Files((FileSystemOptions()));
+  SourceManager SM(Diagnostics, Files);
+
+  llvm::ErrorOr<std::unique_ptr<MemoryBuffer>> Buffer =
+      SM.getFileManager().getBufferForFile(File);
+  if (!Buffer)
+    return errorCodeToError(Buffer.getError());
+  return tooling::applyAtomicChanges(File, Buffer.get()->getBuffer(), Changes,
+                                     Spec);
 }
 
 bool deleteReplacementFiles(const TUReplacementFiles &Files,
                             clang::DiagnosticsEngine &Diagnostics) {
   bool Success = true;
   for (const auto &Filename : Files) {
     std::error_code Error = llvm::sys::fs::remove(Filename);
     if (Error) {

clang-tools-extra/trunk/clang-apply-replacements/tool/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_executable(clang-apply-replacements
   ClangApplyReplacementsMain.cpp
   )
 target_link_libraries(clang-apply-replacements
   PRIVATE
   clangApplyReplacements
   clangBasic
   clangFormat
   clangRewrite
   clangToolingCore
+  clangToolingRefactor
   )
 
 install(TARGETS clang-apply-replacements
   RUNTIME DESTINATION bin)

clang-tools-extra/trunk/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp

private:
   clang::DiagnosticsEngine &Diag;
 };
 } // namespace
 
 static void printVersion(raw_ostream &OS) {
   OS << "clang-apply-replacements version " CLANG_VERSION_STRING << "\n";
 }
 
-/// \brief Convenience function to get rewritten content for \c Filename from
-/// \c Rewrites.
-///
-/// \pre Replacements[i].getFilePath() == Replacements[i+1].getFilePath().
-/// \post Replacements.empty() -> Result.empty()
-///
-/// \param[in] Replacements Replacements to apply
-/// \param[in] Rewrites Rewriter to use to apply replacements.
-/// \param[out] Result Contents of the file after applying replacements if
-/// replacements were provided.
-///
-/// \returns \parblock
-///          \li true if all replacements were applied successfully.
-///          \li false if at least one replacement failed to apply.
-static bool
-getRewrittenData(const std::vector<tooling::Replacement> &Replacements,
-                 Rewriter &Rewrites, std::string &Result) {
-  if (Replacements.empty())
-    return true;
-
-  if (!applyAllReplacements(Replacements, Rewrites))
-    return false;
-
-  SourceManager &SM = Rewrites.getSourceMgr();
-  FileManager &Files = SM.getFileManager();
-
-  StringRef FileName = Replacements.begin()->getFilePath();
-  const clang::FileEntry *Entry = Files.getFile(FileName);
-  assert(Entry && "Expected an existing file");
-  FileID ID = SM.translateFile(Entry);
-  assert(ID.isValid() && "Expected a valid FileID");
-  const RewriteBuffer *Buffer = Rewrites.getRewriteBufferFor(ID);
-  Result = std::string(Buffer->begin(), Buffer->end());
-
-  return true;
-}
-
-/// \brief Apply \c Replacements and return the new file contents.
-///
-/// \pre Replacements[i].getFilePath() == Replacements[i+1].getFilePath().
-/// \post Replacements.empty() -> Result.empty()
-///
-/// \param[in] Replacements Replacements to apply.
-/// \param[out] Result Contents of the file after applying replacements if
-/// replacements were provided.
-/// \param[in] Diagnostics For diagnostic output.
-///
-/// \returns \parblock
-///          \li true if all replacements applied successfully.
-///          \li false if at least one replacement failed to apply.
-static bool
-applyReplacements(const std::vector<tooling::Replacement> &Replacements,
-                  std::string &Result, DiagnosticsEngine &Diagnostics) {
-  FileManager Files((FileSystemOptions()));
-  SourceManager SM(Diagnostics, Files);
-  Rewriter Rewrites(SM, LangOptions());
-
-  return getRewrittenData(Replacements, Rewrites, Result);
-}
-
-/// \brief Apply code formatting to all places where replacements were made.
-///
-/// \pre !Replacements.empty().
-/// \pre Replacements[i].getFilePath() == Replacements[i+1].getFilePath().
-/// \pre Replacements[i].getOffset() <= Replacements[i+1].getOffset().
-///
-/// \param[in] Replacements Replacements that were made to the file. Provided
-/// to indicate where changes were made.
-/// \param[in] FileData The contents of the file \b after \c Replacements have
-/// been applied.
-/// \param[out] FormattedFileData The contents of the file after reformatting.
-/// \param[in] FormatStyle Style to apply.
-/// \param[in] Diagnostics For diagnostic output.
-///
-/// \returns \parblock
-///          \li true if reformatting replacements were all successfully
-///          applied.
-///          \li false if at least one reformatting replacement failed to apply.
-static bool
-applyFormatting(const std::vector<tooling::Replacement> &Replacements,
-                const StringRef FileData, std::string &FormattedFileData,
-                const format::FormatStyle &FormatStyle,
-                DiagnosticsEngine &Diagnostics) {
-  assert(!Replacements.empty() && "Need at least one replacement");
-
-  RangeVector Ranges = calculateChangedRanges(Replacements);
-
-  StringRef FileName = Replacements.begin()->getFilePath();
-  tooling::Replacements R =
-      format::reformat(FormatStyle, FileData, Ranges, FileName);
-
-  // FIXME: Remove this copy when tooling::Replacements is implemented as a
-  // vector instead of a set.
-  std::vector<tooling::Replacement> FormattingReplacements;
-  std::copy(R.begin(), R.end(), back_inserter(FormattingReplacements));
-
-  if (FormattingReplacements.empty()) {
-    FormattedFileData = FileData;
-    return true;
-  }
-
-  FileManager Files((FileSystemOptions()));
-  SourceManager SM(Diagnostics, Files);
-  SM.overrideFileContents(Files.getFile(FileName),
-                          llvm::MemoryBuffer::getMemBufferCopy(FileData));
-  Rewriter Rewrites(SM, LangOptions());
-
-  return getRewrittenData(FormattingReplacements, Rewrites, FormattedFileData);
-}
-
 int main(int argc, char **argv) {
   cl::HideUnrelatedOptions(makeArrayRef(VisibleCategories));
 
   cl::SetVersionPrinter(printVersion);
   cl::ParseCommandLineOptions(argc, argv);
 
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
   DiagnosticsEngine Diagnostics(
int main(int argc, char **argv) {
   // command line option) when exiting main().
   std::unique_ptr<ScopedFileRemover> Remover;
   if (RemoveTUReplacementFiles)
     Remover.reset(new ScopedFileRemover(TUFiles, Diagnostics));
 
   FileManager Files((FileSystemOptions()));
   SourceManager SM(Diagnostics, Files);
 
-  FileToReplacementsMap GroupedReplacements;
-  if (!mergeAndDeduplicate(TURs, GroupedReplacements, SM))
+  FileToChangesMap Changes;
+  if (!mergeAndDeduplicate(TURs, TUDs, Changes, SM))
     return 1;
-  if (!mergeAndDeduplicate(TUDs, GroupedReplacements, SM))
-    return 1;
-
-  Rewriter ReplacementsRewriter(SM, LangOptions());
-
-  for (const auto &FileAndReplacements : GroupedReplacements) {
-    // This shouldn't happen but if a file somehow has no replacements skip to
-    // next file.
-    if (FileAndReplacements.second.empty())
-      continue;
 
-    std::string NewFileData;
-    StringRef FileName = FileAndReplacements.first->getName();
-    if (!applyReplacements(FileAndReplacements.second, NewFileData,
-                           Diagnostics)) {
-      errs() << "Failed to apply replacements to " << FileName << "\n";
-      continue;
-    }
-
-    // Apply formatting if requested.
-    if (DoFormat &&
-        !applyFormatting(FileAndReplacements.second, NewFileData, NewFileData,
-                         FormatStyle, Diagnostics)) {
-      errs() << "Failed to apply reformatting replacements for " << FileName
-             << "\n";
+  tooling::ApplyChangesSpec Spec;
+  Spec.Cleanup = true;
+  Spec.Style = FormatStyle;
+  Spec.Format = DoFormat ? tooling::ApplyChangesSpec::kAll
+                         : tooling::ApplyChangesSpec::kNone;
+
+  for (const auto &FileChange : Changes) {
+    const FileEntry *Entry = FileChange.first;
+    StringRef FileName = Entry->getName();
+    llvm::Expected<std::string> NewFileData =
+        applyChanges(FileName, FileChange.second, Spec, Diagnostics);
+    if (!NewFileData) {
+      errs() << llvm::toString(NewFileData.takeError()) << "\n";
       continue;
     }
 
     // Write new file to disk
     std::error_code EC;
     llvm::raw_fd_ostream FileStream(FileName, EC, llvm::sys::fs::F_None);
     if (EC) {
       llvm::errs() << "Could not open " << FileName << " for writing\n";
       continue;
     }
-
-    FileStream << NewFileData;
+    FileStream << *NewFileData;
   }
 
   return 0;
 }

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/basic/file2.yaml

 ---
 MainSourceFile:     source2.cpp
 Diagnostics:
   - DiagnosticName: test-basic
     Message: Fix
     FilePath: $(path)/basic.h
     FileOffset: 148
     Replacements:
-      - FilePath:        $(path)/basic.h
-        Offset:          148
-        Length:          0
-        ReplacementText: 'override '
+      - FilePath:        $(path)/../basic/basic.h
+        Offset:          298
+        Length:          1
+        ReplacementText: elem
 ...

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/conflict/expected.txt

-There are conflicting changes to $(path)/common.h:
-The following changes conflict:
-  Replace 8:8-8:33 with "auto & i : ints"
-  Replace 8:8-8:33 with "int & elem : ints"
-The following changes conflict:
-  Replace 9:5-9:11 with "elem"
-  Replace 9:5-9:11 with "i"
-The following changes conflict:
-  Remove 12:3-12:14
-  Insert at 12:12 (int*)
-  Replace 12:12-12:12 with "nullptr"
+The new replacement overlaps with an existing replacement.
+New replacement: $(path)/common.h: 106:+26:"int & elem : ints"
+Existing replacement: $(path)/common.h: 106:+26:"auto & i : ints"
+The new replacement overlaps with an existing replacement.
+New replacement: $(path)/common.h: 140:+7:"i"
+Existing replacement: $(path)/common.h: 140:+7:"elem"
+The new replacement overlaps with an existing replacement.
+New replacement: $(path)/common.h: 169:+0:"(int*)"
+Existing replacement: $(path)/common.h: 160:+12:""
+The new replacement overlaps with an existing replacement.
+New replacement: $(path)/common.h: 169:+1:"nullptr"
+Existing replacement: $(path)/common.h: 160:+12:""

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file1.yaml

+---
+MainSourceFile:     identical.cpp
+Diagnostics:
+  - DiagnosticName: test-identical-insertion
+    Message: Fix
+    FilePath: $(path)/identical.cpp
+    FileOffset: 12
+    Replacements:
+      - FilePath:        $(path)/identical.cpp
+        Offset:          12
+        Length:          0
+        ReplacementText: '0'
+      - FilePath:        $(path)/identical.cpp
+        Offset:          12
+        Length:          0
+        ReplacementText: '0'
+...
+

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/identical.cpp

+class MyType {};
+// CHECK: class MyType00 {};

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/order-dependent/expected.txt

+The new insertion has the same insert location as an existing replacement.
+New replacement: $(path)/order-dependent.cpp: 12:+0:"1"
+Existing replacement: $(path)/order-dependent.cpp: 12:+0:"0"

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/order-dependent/file1.yaml

+---
+MainSourceFile:     order-dependent.cpp
+Diagnostics:
+  - DiagnosticName: test-order-dependent-insertion
+    Message: Fix
+    FilePath: $(path)/order-dependent.cpp
+    FileOffset: 12
+    Replacements:
+      - FilePath:        $(path)/order-dependent.cpp
+        Offset:          12
+        Length:          0
+        ReplacementText: '0'
+...

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/order-dependent/file2.yaml

+---
+MainSourceFile:     order-dependent.cpp
+Diagnostics:
+  - DiagnosticName: test-order-dependent-insertion
+    Message: Fix
+    FilePath: $(path)/order-dependent.cpp
+    FileOffset: 12
+    Replacements:
+      - FilePath:        $(path)/order-dependent.cpp
+        Offset:          12
+        Length:          0
+        ReplacementText: '1'
+...

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/order-dependent/order-dependent.cpp

+class MyType {};

clang-tools-extra/trunk/test/clang-apply-replacements/identical.cpp

+// RUN: mkdir -p %T/Inputs/identical
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/identical/identical.cpp > %T/Inputs/identical/identical.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/identical#" %S/Inputs/identical/file1.yaml > %T/Inputs/identical/file1.yaml
+// RUN: clang-apply-replacements %T/Inputs/identical
+// RUN: FileCheck -input-file=%T/Inputs/identical/identical.cpp %S/Inputs/identical/identical.cpp

clang-tools-extra/trunk/test/clang-apply-replacements/order-dependent.cpp

+// RUN: mkdir -p %T/Inputs/order-dependent
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/order-dependent/order-dependent.cpp > %T/Inputs/order-dependent/order-dependent.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/order-dependent#" %S/Inputs/order-dependent/file1.yaml > %T/Inputs/order-dependent/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/order-dependent#" %S/Inputs/order-dependent/file2.yaml > %T/Inputs/order-dependent/file2.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/order-dependent#" %S/Inputs/order-dependent/expected.txt > %T/Inputs/order-dependent/expected.txt
+// RUN: not clang-apply-replacements %T/Inputs/order-dependent > %T/Inputs/order-dependent/output.txt 2>&1
+// RUN: diff -b %T/Inputs/order-dependent/output.txt %T/Inputs/order-dependent/expected.txt

clang-tools-extra/trunk/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp

 //===- clang-apply-replacements/ApplyReplacementsTest.cpp
 //----------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang-apply-replacements/Tooling/ApplyReplacements.h"
+#include "clang/Format/Format.h"
 #include "gtest/gtest.h"
 
 using namespace clang::replace;
 using namespace llvm;
 
 namespace clang {
 namespace tooling {
 

 // Test to ensure diagnostics with no fixes, will be merged correctly
 // before applying.
 TEST(ApplyReplacementsTest, mergeDiagnosticsWithNoFixes) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
   DiagnosticsEngine Diagnostics(
       IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts.get());
   FileManager Files((FileSystemOptions()));
   SourceManager SM(Diagnostics, Files);
+  TUReplacements TURs;
   TUDiagnostics TUs =
       makeTUDiagnostics("path/to/source.cpp", "diagnostic", {}, {}, "path/to");
-  FileToReplacementsMap ReplacementsMap;
+  FileToChangesMap ReplacementsMap;
 
-  EXPECT_TRUE(mergeAndDeduplicate(TUs, ReplacementsMap, SM));
+  EXPECT_TRUE(mergeAndDeduplicate(TURs, TUs, ReplacementsMap, SM));
   EXPECT_TRUE(ReplacementsMap.empty());
 }
 
 } // end namespace tooling
 } // end namespace clang

clang-tools-extra/trunk/unittests/clang-apply-replacements/CMakeLists.txt

 get_filename_component(ClangApplyReplacementsLocation
   "${CMAKE_CURRENT_SOURCE_DIR}/../../clang-apply-replacements/include" REALPATH)
 get_filename_component(CommonIncLocation
   "${CMAKE_CURRENT_SOURCE_DIR}/../include" REALPATH)
 include_directories(
   ${ClangApplyReplacementsLocation}
   ${CommonIncLocation}
   )
 
 add_extra_unittest(ClangApplyReplacementsTests
   ApplyReplacementsTest.cpp
-  ReformattingTest.cpp
   )
 
 target_link_libraries(ClangApplyReplacementsTests
   PRIVATE
   clangApplyReplacements
   clangBasic
   clangToolingCore
+  clangToolingRefactor
   )

clang-tools-extra/trunk/unittests/clang-apply-replacements/ReformattingTest.cpp

-//===- clang-apply-replacements/ReformattingTest.cpp ----------------------===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-#include "clang-apply-replacements/Tooling/ApplyReplacements.h"
-#include "common/VirtualFileHelper.h"
-#include "clang/Format/Format.h"
-#include "clang/Tooling/Refactoring.h"
-#include "gtest/gtest.h"
-
-using namespace clang;
-using namespace clang::tooling;
-using namespace clang::replace;
-
-typedef std::vector<clang::tooling::Replacement> ReplacementsVec;
-
-static Replacement makeReplacement(unsigned Offset, unsigned Length,
-                                   unsigned ReplacementLength,
-                                   llvm::StringRef FilePath = "") {
-  return Replacement(FilePath, Offset, Length,
-                     std::string(ReplacementLength, '~'));
-}
-
-// generate a set of replacements containing one element
-static ReplacementsVec makeReplacements(unsigned Offset, unsigned Length,
-                                        unsigned ReplacementLength,
-                                        llvm::StringRef FilePath = "~") {
-  ReplacementsVec Replaces;
-  Replaces.push_back(
-      makeReplacement(Offset, Length, ReplacementLength, FilePath));
-  return Replaces;
-}
-
-// Put these functions in the clang::tooling namespace so arg-dependent name
-// lookup finds these functions for the EXPECT_EQ macros below.
-namespace clang {
-namespace tooling {
-
-std::ostream &operator<<(std::ostream &os, const Range &R) {
-  return os << "Range(" << R.getOffset() << ", " << R.getLength() << ")";
-}
-
-} // namespace tooling
-} // namespace clang
-
-// Ensure zero-length ranges are produced. Even lines where things are deleted
-// need reformatting.
-TEST(CalculateChangedRangesTest, producesZeroLengthRange) {
-  RangeVector Changes = calculateChangedRanges(makeReplacements(0, 4, 0));
-  EXPECT_EQ(Range(0, 0), Changes.front());
-}
-
-// Basic test to ensure replacements turn into ranges properly.
-TEST(CalculateChangedRangesTest, calculatesRanges) {
-  ReplacementsVec R;
-  R.push_back(makeReplacement(2, 0, 3));
-  R.push_back(makeReplacement(5, 2, 4));
-  RangeVector Changes = calculateChangedRanges(R);
-
-  Range ExpectedRanges[] = { Range(2, 3), Range(8, 4) };
-  EXPECT_TRUE(std::equal(Changes.begin(), Changes.end(), ExpectedRanges));
-}