[PATCH] D18551: Added Fixer implementation and fix() interface in clang-format for removing redundant code.

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: include/clang/Format/Format.h:800
@@ -793,1 +799,3 @@
 
+/// \brief Fix any erroneous/redundant code in the given \p Ranges in \p Code.
+///
----------------
Same as above, "fix" is probably not the right word. "cleanup" seems somewhat better. Here and everywhere else in this patch :-/.

================
Comment at: lib/Format/Format.cpp:1446
@@ -1445,3 +1445,3 @@
 
-class Formatter : public UnwrappedLineConsumer {
+class RangeManager {
 public:
----------------
Maybe call this AffectedRangeManager? That is the class's very specific purpose.

================
Comment at: lib/Format/Format.cpp:1590
@@ +1589,3 @@
+
+class CodeProcesser : public UnwrappedLineConsumer {
+public:
----------------
ioeric wrote:
> I'm not quite sure if the name is accurate. Do you have a better name?
I think the difficulty of finding a name somewhat stems from the fact that it does multiple things at once. I think the main purpose is to encapsulate the "flattening" of the different #if/#else branches in successive runs.

Manuel, do you have an idea for a good name here?

================
Comment at: lib/Format/Format.cpp:1607
@@ -1462,2 +1606,3 @@
 
-  tooling::Replacements format(bool *IncompleteFormat) {
+  tooling::Replacements format(bool *IncompleteFormat);
+
----------------
It feels like these two functions increase the coupling between Formatter/Fixer and CodeProcesser and aren't really necessary. Can you just inline them at the locations where they are called?

================
Comment at: lib/Format/Format.cpp:1669
@@ +1668,3 @@
+
+  friend class Formatter;
+  friend class Fixer;
----------------
Lets not have this access. Provide getters.

================
Comment at: lib/Format/Format.cpp:1908
@@ +1907,3 @@
+  int checkEmptyNamespace(unsigned CurrentLine) {
+    if (AnnotatedLines[CurrentLine]->NamespaceEmptynessChecked)
+      return -1;
----------------
I see why you are doing it this way, but let me propose something else:

How about iterating over all of the lines for each thing to cleanup (namespaces, ctor-intializers, etc.). I don't think it makes a significant difference in the runtime. Then you can just skip all the lines that you have already looked at (i.e. the returned int).

Among other things, you can probably remove the duplication of the AnnotatedLines[CurrentLine]->startsWith(..) and maybe even the AnnotatedLine::Deleted field.

================
Comment at: lib/Format/FormatToken.h:283
@@ +282,3 @@
+  // \brief Fixer will set this to true if the token is going to be deleted.
+  bool Deleted = false;
+
----------------
Do you need this? Wouldn't it be enough to add them to the set of deleted tokens and mark them as Finalized?


http://reviews.llvm.org/D18551