klimek added inline comments.

================
Comment at: include/clang/Format/Format.h:769
@@ -768,1 +768,3 @@
 
+/// \brief Returns the replacements corresponding to applying \p Replaces and
+/// cleaning up the code after that.
----------------
cleanupAroundReplacements sounds good.

================
Comment at: lib/Format/Format.cpp:1466
@@ -1464,1 +1465,3 @@
+  template <typename T>
+  tooling::Replacements process(T Callable, bool *IncompleteFormat) {
     tooling::Replacements Result;
----------------
Nit: I think it's idiomatic to call this a "Callback".

================
Comment at: lib/Format/Format.cpp:2143
@@ +2142,3 @@
+
+  CodeProcessor Processor(Expanded, SourceMgr, ID, Ranges);
+  auto Callable =
----------------
This is unexpected: I'd not have expected the CodeProcessor to be used to keep 
state apart from what's used during a Process() run. I think we'll need a 
different name.

================
Comment at: lib/Format/Format.cpp:2176
@@ +2175,3 @@
+                   bool *IncompleteFormat) -> tooling::Replacements {
+    Cleaner Cleaner(Processor);
+    return Cleaner.runCleanup(Tokens, AnnotatedLines);
----------------
The way the Processor is passed into the callbacks so they can call stuff on 
the processor again makes me think this is really tightly coupled and should 
use inheritance.

Alternatively, we might want to try to break up the CodeProcessor into a part 
that just runs the callback and keeps the state for running the callback 
around, and an interface that is used by the callbacks - I'll need to look more 
closely into this though.


http://reviews.llvm.org/D18551



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to