[PATCH] D29300: [clang-format] Refactor WhitespaceManager and friends
djasper marked an inline comment as done.
djasper added inline comments.
Comment at: lib/Format/WhitespaceManager.cpp:178
- LastBlockComment =
-} else if (Change.Kind == tok::unknown) {
- if ((Change.StartOfBlockComment = LastBlockComment))
krasimir wrote:
> What happened to the tok::unknown case?
We actually don't set the token back to unknown in replaceWhitespaceInToken
anymore if this is a comment. So I moved this logic back into the if above.
https://reviews.llvm.org/D29300
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29300: [clang-format] Refactor WhitespaceManager and friends
krasimir added a comment. looks good! https://reviews.llvm.org/D29300 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29300: [clang-format] Refactor WhitespaceManager and friends
krasimir added inline comments.
Comment at: lib/Format/WhitespaceManager.cpp:178
- LastBlockComment =
-} else if (Change.Kind == tok::unknown) {
- if ((Change.StartOfBlockComment = LastBlockComment))
What happened to the tok::unknown case?
https://reviews.llvm.org/D29300
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29300: [clang-format] Refactor WhitespaceManager and friends
djasper closed this revision. djasper added a comment. Submitted as r293616. https://reviews.llvm.org/D29300 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29300: [clang-format] Refactor WhitespaceManager and friends
djasper updated this revision to Diff 86404.
djasper added a comment.
Added assert. Removed unused InToken.
https://reviews.llvm.org/D29300
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/ContinuationIndenter.cpp
lib/Format/ContinuationIndenter.h
lib/Format/FormatToken.h
lib/Format/TokenAnnotator.cpp
lib/Format/UnwrappedLineFormatter.cpp
lib/Format/UnwrappedLineFormatter.h
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -43,8 +43,7 @@
/// \brief Replaces the whitespace in front of \p Tok. Only call once for
/// each \c AnnotatedToken.
- void replaceWhitespace(FormatToken , unsigned Newlines,
- unsigned IndentLevel, unsigned Spaces,
+ void replaceWhitespace(FormatToken , unsigned Newlines, unsigned Spaces,
unsigned StartOfTokenColumn,
bool InPPDirective = false);
@@ -72,8 +71,7 @@
unsigned ReplaceChars,
StringRef PreviousPostfix,
StringRef CurrentPrefix, bool InPPDirective,
-unsigned Newlines, unsigned IndentLevel,
-int Spaces);
+unsigned Newlines, int Spaces);
/// \brief Returns all the \c Replacements created during formatting.
const tooling::Replacements ();
@@ -91,8 +89,6 @@
const SourceManager
};
-Change() {}
-
/// \brief Creates a \c Change.
///
/// The generated \c Change will replace the characters at
@@ -102,12 +98,17 @@
///
/// \p StartOfTokenColumn and \p InPPDirective will be used to lay out
/// trailing comments and escaped newlines.
-Change(bool CreateReplacement, SourceRange OriginalWhitespaceRange,
- unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
- unsigned NewlinesBefore, StringRef PreviousLinePostfix,
- StringRef CurrentLinePrefix, tok::TokenKind Kind,
- bool ContinuesPPDirective, bool IsStartOfDeclName,
- bool IsInsideToken);
+Change(const FormatToken , bool CreateReplacement,
+ SourceRange OriginalWhitespaceRange, int Spaces,
+ unsigned StartOfTokenColumn, unsigned NewlinesBefore,
+ StringRef PreviousLinePostfix, StringRef CurrentLinePrefix,
+ bool ContinuesPPDirective, bool IsInsideToken);
+
+// The kind of the token whose whitespace this change replaces, or in which
+// this change inserts whitespace.
+// FIXME: Currently this is not set correctly for breaks inside comments, as
+// the \c BreakableToken is still doing its own alignment.
+const FormatToken *Tok;
bool CreateReplacement;
// Changes might be in the middle of a token, so we cannot just keep the
@@ -117,18 +118,7 @@
unsigned NewlinesBefore;
std::string PreviousLinePostfix;
std::string CurrentLinePrefix;
-// The kind of the token whose whitespace this change replaces, or in which
-// this change inserts whitespace.
-// FIXME: Currently this is not set correctly for breaks inside comments, as
-// the \c BreakableToken is still doing its own alignment.
-tok::TokenKind Kind;
bool ContinuesPPDirective;
-bool IsStartOfDeclName;
-
-// The number of nested blocks the token is in. This is used to add tabs
-// only for the indentation, and not for alignment, when
-// UseTab = US_ForIndentation.
-unsigned IndentLevel;
// The number of spaces in front of the token or broken part of the token.
// This will be adapted when aligning tokens.
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -25,64 +25,60 @@
C2.OriginalWhitespaceRange.getBegin());
}
-WhitespaceManager::Change::Change(
-bool CreateReplacement, SourceRange OriginalWhitespaceRange,
-unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
-unsigned NewlinesBefore, StringRef PreviousLinePostfix,
-StringRef CurrentLinePrefix, tok::TokenKind Kind, bool ContinuesPPDirective,
-bool IsStartOfDeclName, bool IsInsideToken)
-: CreateReplacement(CreateReplacement),
+WhitespaceManager::Change::Change(const FormatToken ,
+ bool CreateReplacement,
+ SourceRange OriginalWhitespaceRange,
+ int Spaces, unsigned StartOfTokenColumn,
+ unsigned NewlinesBefore,
+ StringRef PreviousLinePostfix,
+ StringRef CurrentLinePrefix,
+
[PATCH] D29300: [clang-format] Refactor WhitespaceManager and friends
klimek added a comment.
Generally looks like the right direction, minus that I'm not sure yet what the
plan for things broken in BreakableToken are.
Comment at: lib/Format/TokenAnnotator.cpp:1843
+Current->MatchingParen->opensBlockOrBlockTypeList(Style))
+ --IndentLevel;
+Current->IndentLevel = IndentLevel;
Perhaps add an assert that we don't underflow.
Comment at: lib/Format/UnwrappedLineFormatter.cpp:904-907
+void UnwrappedLineFormatter::formatFirstToken(const AnnotatedLine ,
const AnnotatedLine
*PreviousLine,
- unsigned IndentLevel,
- unsigned Indent,
- bool InPPDirective) {
+ unsigned Indent) {
+ FormatToken& RootToken = *Line.First;
I'm not sure I understand the change in the function signature. Given that we
really only need InPPDirective and FirstToken, it seems unnecessary to hand in
the whole line? (in the spirit of minimal interfaces)
Comment at: lib/Format/WhitespaceManager.h:109-110
+// this change inserts whitespace.
+// FIXME: Currently this is not set correctly for breaks inside comments,
as
+// the \c BreakableToken is still doing its own alignment.
+const FormatToken *Tok;
What's the proposed fix?
https://reviews.llvm.org/D29300
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29300: [clang-format] Refactor WhitespaceManager and friends
djasper created this revision.
The main motivation behind this is to cleanup the WhitespaceManager and make it
more extensible for future alignment etc. features. Specifically,
WhitespaceManager has started to copy more and more code that is already
present in FormatToken. Instead, I think it makes more sense to actually store
a reference to each FormatToken for each change.
This has as a consequence led to a change in the calculation of indent levels.
Now, we actually compute them for each Token ahead of time, which should be
more efficient as it removes an unsigned value for the ParenState, which is
used during the combinatorial exploration of the solution space.
No functional changes intended.
https://reviews.llvm.org/D29300
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/ContinuationIndenter.cpp
lib/Format/ContinuationIndenter.h
lib/Format/FormatToken.h
lib/Format/TokenAnnotator.cpp
lib/Format/UnwrappedLineFormatter.cpp
lib/Format/UnwrappedLineFormatter.h
lib/Format/WhitespaceManager.cpp
lib/Format/WhitespaceManager.h
unittests/Format/FormatTestJS.cpp
Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -497,6 +497,11 @@
"[];");
verifyFormat("someFunction([], {a: a});");
+
+ verifyFormat("var string = [\n"
+ " 'aa',\n"
+ " 'bb',\n"
+ "].join('+');");
}
TEST_F(FormatTestJS, ColumnLayoutForArrayLiterals) {
@@ -587,6 +592,11 @@
" doSomething();\n"
"}, this));");
+ verifyFormat("SomeFunction(function() {\n"
+ " foo();\n"
+ " bar();\n"
+ "}.bind(this));");
+
// FIXME: This is bad, we should be wrapping before "function() {".
verifyFormat("someFunction(function() {\n"
" doSomething(); // break\n"
Index: lib/Format/WhitespaceManager.h
===
--- lib/Format/WhitespaceManager.h
+++ lib/Format/WhitespaceManager.h
@@ -43,8 +43,7 @@
/// \brief Replaces the whitespace in front of \p Tok. Only call once for
/// each \c AnnotatedToken.
- void replaceWhitespace(FormatToken , unsigned Newlines,
- unsigned IndentLevel, unsigned Spaces,
+ void replaceWhitespace(FormatToken , unsigned Newlines, unsigned Spaces,
unsigned StartOfTokenColumn,
bool InPPDirective = false);
@@ -72,8 +71,7 @@
unsigned ReplaceChars,
StringRef PreviousPostfix,
StringRef CurrentPrefix, bool InPPDirective,
-unsigned Newlines, unsigned IndentLevel,
-int Spaces);
+unsigned Newlines, int Spaces);
/// \brief Returns all the \c Replacements created during formatting.
const tooling::Replacements ();
@@ -91,8 +89,6 @@
const SourceManager
};
-Change() {}
-
/// \brief Creates a \c Change.
///
/// The generated \c Change will replace the characters at
@@ -102,12 +98,18 @@
///
/// \p StartOfTokenColumn and \p InPPDirective will be used to lay out
/// trailing comments and escaped newlines.
-Change(bool CreateReplacement, SourceRange OriginalWhitespaceRange,
- unsigned IndentLevel, int Spaces, unsigned StartOfTokenColumn,
- unsigned NewlinesBefore, StringRef PreviousLinePostfix,
- StringRef CurrentLinePrefix, tok::TokenKind Kind,
- bool ContinuesPPDirective, bool IsStartOfDeclName,
- bool IsInsideToken);
+Change(const FormatToken , bool CreateReplacement,
+ SourceRange OriginalWhitespaceRange, int Spaces,
+ unsigned StartOfTokenColumn, unsigned NewlinesBefore,
+ StringRef PreviousLinePostfix, StringRef CurrentLinePrefix,
+ bool ContinuesPPDirective, bool IsInsideToken);
+
+// The kind of the token whose whitespace this change replaces, or in which
+// this change inserts whitespace.
+// FIXME: Currently this is not set correctly for breaks inside comments, as
+// the \c BreakableToken is still doing its own alignment.
+const FormatToken *Tok;
+bool InToken;
bool CreateReplacement;
// Changes might be in the middle of a token, so we cannot just keep the
@@ -117,18 +119,7 @@
unsigned NewlinesBefore;
std::string PreviousLinePostfix;
std::string CurrentLinePrefix;
-// The kind of the token whose whitespace this change replaces, or in which
-// this change inserts whitespace.
-// FIXME: Currently this is not set correctly for breaks inside comments, as
-// the \c BreakableToken is still doing its own alignment.
-tok::TokenKind
