[PATCH] Introducing new tool clang-replace

Manuel Klimek klimek at google.com
Fri Aug 16 09:23:18 PDT 2013


  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



More information about the cfe-commits mailing list