[cfe-commits] [PATCH] Initial version of formatting library

[Dmitri Gribenko]

  So, in a nutshell, this is going to become a forgiving C++ parser?  E.g., it will handle macro invocations in unexpanded form, just assuming that it will expand to something sane and we will be able to resynchronize?

  All these addContinuation() calls essentially hardcode pretty-printing policy.  It might make sense to add them everywhere where some style specifies to put a line break, and after that enable them conditionally based on some sort of policy.  Does this make sense?


================
Comment at: unittests/Format/FormatTest.cpp:122
@@ +121,3 @@
+      format("if(true)\nif(true)\nif(true)\nf();\n"
+             "else\ng();\nelse\nh();\nelse\ni();", 0, 1));
+  EXPECT_EQ(
----------------
It is unfortunate that these testcases have duplicated strings.  Here's a trick (you might not like it, it is just a suggestion): add function "AssertionResult checkFormat(char *)".  It will first strip whitespace from the string, pass it to format() and compare results.  This will not work for code with comments, but there will be few of those testcases.


================
Comment at: lib/Format/ContinuationParser.cpp:171
@@ +170,3 @@
+  Cont.Tokens.push_back(FormatTok);
+  return parseToken();
+}
----------------
'return' is not required since return type is void.

================
Comment at: lib/Format/ContinuationParser.cpp:167
@@ +166,3 @@
+
+void ContinuationParser::nextToken() {
+  if (eof())
----------------
Bikeshedding: I would call it 'consumeToken()'

================
Comment at: lib/Format/ContinuationParser.cpp:174
@@ +173,3 @@
+
+void ContinuationParser::parseToken() {
+  FormatTok = FormatToken();
----------------
Bikeshedding: I would call it 'populateLookahead()'

================
Comment at: lib/Format/ContinuationParser.cpp:61
@@ +60,3 @@
+  --Cont.Level;
+  if (FormatTok.Tok.getKind() != tok::r_brace) abort();
+  nextToken();
----------------
assert()...

But is this always true?  It may be EOF already (testcase: "{{}")  We should decide how do we handle invalid code.  But definitely we should not assert.

================
Comment at: lib/Format/Format.cpp:45
@@ +44,3 @@
+    State.Indent.push_back(Cont.Level * 2 + 4);
+    for (unsigned i = 1; i < Cont.Tokens.size(); ++i) {
+      bool InsertNewLine = Cont.Tokens[i].NewlinesBefore > 0;
----------------
..., e = Cont.Tokens.size(); i != e; ++i ?

================
Comment at: lib/Format/ContinuationParser.cpp:129
@@ +128,3 @@
+void ContinuationParser::parseIfThenElse() {
+  if (FormatTok.Tok.getKind() != tok::raw_identifier) abort();
+  nextToken();
----------------
assert()

================
Comment at: lib/Format/ContinuationParser.cpp:34
@@ +33,3 @@
+void ContinuationParser::parseLevel() {
+  do {
+    switch(FormatTok.Tok.getKind()) {
----------------
assert(!eof()) and fix up the callers or this should be a while(!eof()){} loop.

And in ContinuationParser::parseStatement(), too.

================
Comment at: lib/Format/ContinuationParser.cpp:70
@@ +69,3 @@
+    nextToken();
+    if (FormatTok.NewlinesBefore > 0) return;
+  }
----------------
This does not handle PP directives with line continuations, right?


================
Comment at: lib/Format/ContinuationParser.cpp:77
@@ +76,3 @@
+    nextToken();
+    if (FormatTok.NewlinesBefore > 0) {
+      addContinuation();
----------------
Comments with line continuations?


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