[PATCH] clang-apply-replacements: Add code-formatting post processing step

Daniel Jasper djasper at google.com
Wed Sep 25 09:04:03 PDT 2013


  Some high-level comments. I think we should first figure out the general direction. Happy to chat, too.


================
Comment at: clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h:38
@@ +37,3 @@
+/// \brief Collection of source ranges.
+typedef std::vector<clang::tooling::Range> RangeVec;
+
----------------
Not a fan of abbreviations. Fan of "tor" :-).

================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:289
@@ +288,3 @@
+
+void calculateChangedRanges(
+    const std::vector<clang::tooling::Replacement> &Replaces,
----------------
I think a lot of the complexity stems from this function accepting non-empty ChangedRanges. I'd prefer to simply impose a restriction until this functionality is actually needed. Or do you already have a use case?

================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:117
@@ +116,3 @@
+/// code.
+static void formatReplacements(const Rewriter &AppliedReplacements,
+                               const FileToReplacementsMap &GroupedReplacements,
----------------
I think this interface is too complex and has too many assumptions. I think it would get significantly easier if it operated on a single file. Does anything speak against moving the "if (DoFormat)" below into the loop.

Also, while you can of course try to reuse rewriters ans SourceManagers, I think this makes not only the interface more complex, but also limits the possibilities for executing this in multiple threads. Why not just hand in the applied replacements and a string with the code?


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



More information about the cfe-commits mailing list