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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
