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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
