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

[Daniel Jasper]

  Thank you for the detailed feedback!


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

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

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

================
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());
----------------
Dmitri Gribenko wrote:
> 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?
> 
Done.

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

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

================
Comment at: lib/Format/Format.cpp:59
@@ +58,3 @@
+  // The current state when indenting a continuation.
+  struct IndentState {
+    unsigned ParenLevel;
----------------
Dmitri Gribenko wrote:
> 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.
There are many of these created (copied by value) during the recursive formatting. This might not be ideal, but is much easier then implementing the inverse to addToken() and it definitely suffices for now. Added comment to clarify.

================
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";
----------------
Dmitri Gribenko wrote:
> assert(State.ParenLevel != 0);
Done. Also, added FIXME because we want to be able to handle invalid code.

================
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
----------------
Dmitri Gribenko wrote:
> Doxygen '///'?
Done.

================
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.
----------------
Dmitri Gribenko wrote:
> If \p NewLine ...
Done.

================
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
----------------
Dmitri Gribenko wrote:
> \param EndIndex ...
Done.

================
Comment at: lib/Format/Format.cpp:113
@@ +112,3 @@
+  // better solution.
+  int numLines(IndentState State, bool NewLine, unsigned Index,
+               unsigned EndIndex, int StopAt) {
----------------
Dmitri Gribenko wrote:
> 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?
Changed to unsigned.

No. It is very important that IndentState is copied by value.

================
Comment at: lib/Format/Format.cpp:140
@@ +139,3 @@
+
+  void setWhitespace(const FormatToken& Tok, unsigned NewLines,
+                     unsigned Spaces) {
----------------
Dmitri Gribenko wrote:
> 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.
Rename to replaceWhitespace and added comment.

================
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) {
----------------
Dmitri Gribenko wrote:
> ... \p Token?
Done.


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