Yes! This is much more like it. I especially like that you used 
DiagnosticsEngine.


================
Comment at: clang-replace/ApplyReplacements.h:35-36
@@ +34,4 @@
+///          \li false If there were conflicts or some other error occurred.
+bool applyChangeDescriptions(const llvm::StringRef Directory,
+                             clang::DiagnosticsEngine &Diagnostics);
+
----------------
I think this function does too much unrelated things.

I'd expect this split up into multiple parts:
- a function that finds all replacement files in a directory, slurps them in, 
and returns a data structure with all replacements for all files
- a function that takes such a data structure and applies the replacements

Depending on the infrastructure you use, you might have very different ways to 
find those replacements - having a public interface that requires you to put 
them into files on a disk seems very restrictive.

================
Comment at: clang-replace/ApplyReplacements.cpp:42-44
@@ +41,5 @@
+/// directory structure.
+static error_code collectReplacementsDocs(const StringRef Directory,
+                                      ReplacementsDocs &ChangeDocs,
+                                      DiagnosticsEngine &Diagnostics) {
+  using namespace llvm::sys::fs;
----------------
clang-format :D

Also, this is probably what I'd just put into the public interface instead of 
hiding it down here, where nobody can observe its beauty ;)


http://llvm-reviews.chandlerc.com/D1424
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to