[PATCH] Refactor clang-format's formatter.
Manuel Klimek
klimek at google.com
Tue May 12 01:39:48 PDT 2015
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:28
@@ -27,1 +27,3 @@
+/// \brief Tracks the indent level for an \c AnnotatedLine across levels.
+struct LevelIndentTracker {
----------------
djasper wrote:
> I think "Tracks the indent level of \c AnnotatedLines across levels" is slightly clearer as this tracks the indent for multiple lines.
Done.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:29
@@ +28,3 @@
+/// \brief Tracks the indent level for an \c AnnotatedLine across levels.
+struct LevelIndentTracker {
+ LevelIndentTracker(const FormatStyle &Style,
----------------
djasper wrote:
> I think this should be a class, not a struct.
Yep.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:34
@@ +33,3 @@
+ : Style(Style), Keywords(Keywords) {
+ for (unsigned i = 0, e = StartLevel; i != e; ++i)
+ IndentForLevel.push_back(Style.IndentWidth * i + AdditionalIndent);
----------------
djasper wrote:
> Just use:
>
> for (unsigned i = 0; i != StartLevel; ++i) ...
>
> StartLevel already is a POD type (unsigned even) and we don't get anything.
Done.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:39-43
@@ +38,7 @@
+ /// \brief Returns the indent for the current line.
+ unsigned getIndent() const { return Indent; }
+
+ /// \brief Update the indent state given that \p Line is going to be formatted
+ /// next.
+ void updateIndentForLine(const AnnotatedLine &Line) {
+ Offset = getIndentOffset(*Line.First);
----------------
djasper wrote:
> Ignore.. Phab doesn't let me delete this comment (Eclipse-like behavior).
Ignored</irony>
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:62
@@ +61,3 @@
+ void updateLevelIndentFromUnmodifiedLine(const AnnotatedLine &Line) {
+ unsigned LevelIndent = Line.First->OriginalColumn;
+ if (static_cast<int>(LevelIndent) - Offset >= 0)
----------------
djasper wrote:
> Isn't this missing an
>
> Offset = getIndentOffset(*Line.First);
Nope. Added class comment and local method comment.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:106-110
@@ +105,7 @@
+
+ /// \brief Offset of the current line relative to the indent level.
+ ///
+ /// For example, the 'public' keywords is often indented with a negative
+ /// offset.
+ int Offset = 0;
+
----------------
djasper wrote:
> As obvious from my comment above where I probably show that I am not understanding something, it is unclear how this needs to be a class variable instead of a variable calculated per line. Please comment.
I hope the additional comments on the methods clarify...
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:132-144
@@ +131,15 @@
+ tryFitMultipleLinesInOne(IndentTracker.getIndent(), Next, End);
+ if (MergedLines > 0 && Style.ColumnLimit == 0) {
+ // Disallow line merging if there is a break at the start of one of the
+ // input lines.
+ for (unsigned i = 0; i < MergedLines; ++i) {
+ if (Next[i + 1]->First->NewlinesBefore > 0)
+ MergedLines = 0;
+ }
+ }
+ if (!DryRun) {
+ for (unsigned i = 0; i < MergedLines; ++i) {
+ join(*Next[i], *Next[i + 1]);
+ }
+ }
+ Next = Next + MergedLines + 1;
----------------
djasper wrote:
> Remove all the braces.
Done. ALL.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:440-441
@@ -308,2 +439,4 @@
const AdditionalKeywords &Keywords;
+ const SmallVectorImpl<AnnotatedLine*>::const_iterator End;
+ SmallVectorImpl<AnnotatedLine*>::const_iterator Next;
};
----------------
djasper wrote:
> Can you change the order? My OCD finds it weird to have Next after End ;-).
The problem is that my OCD finds it weird to have a non-const member in between the const ones. Here, I added an empty line, perhaps that helps?
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:815-819
@@ +814,7 @@
+ bool Format = TheLine.Type != LT_Invalid && ShouldFormat;
+ // If we format a line, each AnnotatedLine will start a new line (otherwise
+ // we merge them into the same AnnotatedLine).
+ // If we do not format a line, this does not hold true - in that case an
+ // arbitrary number of annotated lines can be in one line.
+ bool AnnotatedLineStartsNewLine =
+ TheLine.First->NewlinesBefore > 0 || TheLine.First->IsFirst;
----------------
djasper wrote:
> Both, the comment and the variable name are a bit confusing. Lets discuss this offline.
Resolved via that: moved into the !Format part and rewrote comment.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:829
@@ +828,3 @@
+ if (!Format && ShouldFormat && IncompleteFormat) {
+ assert(TheLine.Type == LT_Invalid);
+ *IncompleteFormat = true;
----------------
djasper wrote:
> This assert seems a bit odd ..
Resolved via chat: moved around and duplicated the condition.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:833
@@ -710,20 +832,3 @@
- bool FixIndentation =
- FixBadIndentation && (LevelIndent != FirstTok->OriginalColumn);
- bool ShouldFormat = TheLine.Affected || FixIndentation;
- if (TheLine.First->is(tok::eof)) {
- if (PreviousLine && PreviousLine->Affected && !DryRun) {
- // Remove the file's trailing whitespace.
- unsigned Newlines = std::min(FirstTok->NewlinesBefore, 1u);
- Whitespaces->replaceWhitespace(*TheLine.First, Newlines,
- /*IndentLevel=*/0, /*Spaces=*/0,
- /*TargetColumn=*/0);
- }
- } else if (TheLine.Type != LT_Invalid && ShouldFormat) {
- if (FirstTok->WhitespaceRange.isValid()) {
- if (!DryRun)
- formatFirstToken(*TheLine.First, PreviousLine, TheLine.Level, Indent,
- TheLine.InPPDirective);
- } else {
- Indent = LevelIndent = FirstTok->OriginalColumn;
- }
+ if (Format) {
+ if (!DryRun)
----------------
djasper wrote:
> The actions for "Format" and "!Format" seem pretty interleaved here. Couldn't we group them more?
Resolved via chat: moved some of the code around.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:843
@@ -738,7 +842,3 @@
- if (TheLine.Last->TotalLength + Indent <= ColumnLimit ||
- TheLine.Type == LT_ImportStatement) {
- Penalty += NoLineBreakFormatter(Indenter, Whitespaces, Style, this)
- .formatLine(TheLine, Indent, DryRun);
- } else if (Style.ColumnLimit == 0) {
+ if (Style.ColumnLimit == 0) {
NoColumnLimitLineFormatter(Indenter, Whitespaces, Style, this)
----------------
djasper wrote:
> No braces.. Here and below.
Done.
================
Comment at: lib/Format/UnwrappedLineFormatter.cpp:895
@@ +894,3 @@
+ Whitespaces->replaceWhitespace(RootToken, Newlines, /*IndentLevel=*/0,
+ /*Spaces=*/0,
+ /*TargetColumn=*/0);
----------------
djasper wrote:
> Merge this line with the next (there already is a clang-format bug for that).
Done.
================
Comment at: lib/Format/UnwrappedLineFormatter.h:53
@@ -68,15 +52,3 @@
- /// \brief Get the indent of \p Level from \p IndentForLevel.
- ///
- /// \p IndentForLevel must contain the indent for the level \c l
- /// at \p IndentForLevel[l], or a value < 0 if the indent for
- /// that level is unknown.
- unsigned getIndent(ArrayRef<int> IndentForLevel, unsigned Level);
-
- void join(AnnotatedLine &A, const AnnotatedLine &B);
-
- unsigned getColumnLimit(bool InPPDirective) const {
- // In preprocessor directives reserve two chars for trailing " \"
- return Style.ColumnLimit - (InPPDirective ? 2 : 0);
- }
+ unsigned getColumnLimit(bool InPPDirective, const AnnotatedLine *NextLine) const;
----------------
djasper wrote:
> If the implementation isn't in the header anymore, this might deserve a small comment.
Yep.
http://reviews.llvm.org/D9662
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the cfe-commits
mailing list