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

[Dmitri Gribenko]

================
Comment at: lib/Format/ContinuationParser.h:24
@@ +23,3 @@
+
+struct FormatToken {
+  FormatToken() : NewlinesBefore(0), WhiteSpaceLength(0) {}
----------------
struct FormatToken and struct Continuation seem to be the most important abstractions.  Please document their members, add examples as appropriate. 

================
Comment at: lib/Format/ContinuationParser.cpp:88
@@ +87,3 @@
+      case tok::semi:
+        {
+          nextToken();
----------------
Usually the brace goes to the previous line.  (But it is not needed here anyway.)

================
Comment at: lib/Format/ContinuationParser.cpp:118
@@ +117,3 @@
+void ContinuationParser::parseParens() {
+  if (FormatTok.Tok.getKind() != tok::l_paren) abort();
+  nextToken();
----------------
Maybe assert?

================
Comment at: lib/Format/ContinuationParser.cpp:148
@@ +147,3 @@
+  if (FormatTok.Tok.getKind() == tok::raw_identifier) {
+    StringRef Data(Sources.getCharacterData(FormatTok.Tok.getLocation()),
+                   FormatTok.Tok.getLength());
----------------
This token text extraction code is used quite a few times in the implementation.  Is there a good way to abstract it so that it doesn't get duplicated?


================
Comment at: lib/Format/Format.cpp:31
@@ +30,3 @@
+public:
+  ContinuationFormatter(SourceManager &Sources,
+                        const Continuation &Cont,
----------------
'Sources' is a bit unusual name for a SourceManager.  Maybe 'SM' or 'SourceMgr'?

================
Comment at: lib/Format/Format.cpp:106
@@ +105,3 @@
+
+  // Calculate the number of lines needed to format the remaining part of the
+  // continuation starting in the state 'State'. If 'NewLine' is set, a new line
----------------
Doxygen '///'?

================
Comment at: lib/Format/Format.cpp:109
@@ +108,3 @@
+  // will be added after the previous token.
+  // 'EndIndex' is the last token belonging to the continuation.
+  // 'StopAt' is used for optimization. If we can determine that we'll
----------------
\param EndIndex ...

================
Comment at: lib/Format/Format.cpp:107
@@ +106,3 @@
+  // Calculate the number of lines needed to format the remaining part of the
+  // continuation starting in the state 'State'. If 'NewLine' is set, a new line
+  // will be added after the previous token.
----------------
If \p NewLine ...

================
Comment at: lib/Format/Format.cpp:113
@@ +112,3 @@
+  // better solution.
+  int numLines(IndentState State, bool NewLine, unsigned Index,
+               unsigned EndIndex, int StopAt) {
----------------
Does this ever return a negative number?  If not, 'unsigned' would be a better result type.  Same for parameter StopAt.

Also, maybe const IndentState &State?

================
Comment at: lib/Format/Format.cpp:174
@@ +173,3 @@
+
+  /// \brief Add a new line before token \c Index.
+  void addNewline(const FormatToken &Token, unsigned Level) {
----------------
... \p Token?

================
Comment at: lib/Format/ContinuationParser.cpp:167
@@ +166,3 @@
+void ContinuationParser::nextToken() {
+  if (eof()) return;
+  Cont.Tokens.push_back(FormatTok);
----------------
One statement per line, please.

================
Comment at: lib/Format/Format.cpp:92
@@ +91,3 @@
+    if (Cont.Tokens[Index].Tok.getKind() == tok::r_paren) {
+      if (State.ParenLevel == 0) {
+        llvm::outs() << "ParenLevel is 0!!!\n";
----------------
assert(State.ParenLevel != 0);

================
Comment at: lib/Format/Format.cpp:140
@@ +139,3 @@
+
+  void setWhitespace(const FormatToken& Tok, unsigned NewLines,
+                     unsigned Spaces) {
----------------
Ampersand goes next to the identifier.

Also, the name of the function suggests that calling it multiple times is harmless (overriding what previous invocations set), but apparently it is not.

================
Comment at: lib/Format/Format.cpp:59
@@ +58,3 @@
+  // The current state when indenting a continuation.
+  struct IndentState {
+    unsigned ParenLevel;
----------------
It seems like that only one object of this type is created during lifetime of this class, and members of this struct are tied to the Continuation being processed.  Does it make sense to just dump all these members into the class?

Also, please document these members.  It is not obvious what these store, especially vectors.


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