[PATCH] D42036: [clang-format] Keep comments aligned to macros
mzeren-vmw added a comment. In https://reviews.llvm.org/D42036#984401, @djasper wrote: > To me, aligning with the define seems fundamentally wrong. we definitely have code that does that internally. It can also be seen in the wild e.g.: https://github.com/boostorg/config/blob/develop/include/boost/config/detail/posix_features.hpp However, it seems reasonable that clang-format's "default" be alignment with #. That will be a simpler patch, and it will resolve the toggling behavior. Let me work that up in a separate review. Repository: rC Clang https://reviews.llvm.org/D42036 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42036: [clang-format] Keep comments aligned to macros
djasper added a comment. While I agree that there is probably a bug to fix, I don't (yet) agree with what is proposed in this patch. I think a comment in between preprocessor directives should always either: - Be considered part of the code in between the #-lines - Be considered to be commenting on the subsequent #-line In the former case, we need to indent with the regular IndentWidth, completely irrespective of anything inside the preprocessor lines. In the latter case, we should align with the # in column 0. To me, aligning with the define seems fundamentally wrong. Repository: rC Clang https://reviews.llvm.org/D42036 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42036: [clang-format] Keep comments aligned to macros
krasimir added inline comments.
Comment at: lib/Format/TokenAnnotator.cpp:1725
+ NextNonComment.Type == LT_ImportStatement) &&
+ NextNonComment.Level > 0 && !Comment.InPPDirective)
+return NextNonComment.First->Next &&
Why are we checking `NextNonComment.Level > 0` here? We could be aligned with
the next preprocessor directive even at level 0.
Comment at: lib/Format/TokenAnnotator.cpp:1729
+NextNonComment.First->Next->OriginalColumn)
+ ? CA_Directive
+ : CA_None;
I think this should be enabled only if preprocessor indentation has been
enabled.
Comment at: lib/Format/TokenAnnotator.cpp:1761
+ if (Alignment == CA_Directive)
+(*I)->LevelOffset = 1;
} else {
Could you add a comment that this `1` comes from the `#` in the preprocessor
directive?
Comment at: unittests/Format/FormatTest.cpp:2629
+ "# define B 0\n"
+ " // Trailing aligned comment\n"
+ "#endif\n"
I would expect this comment to be aligned with the `#endif` on the next line.
Comment at: unittests/Format/FormatTest.cpp:2657
+ " * Trailing aligned comment\n"
+ " */\n"
+ "#endif\n"
Same here.
Repository:
rC Clang
https://reviews.llvm.org/D42036
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42036: [clang-format] Keep comments aligned to macros
mzeren-vmw marked 2 inline comments as done.
mzeren-vmw added inline comments.
Comment at: lib/Format/TokenAnnotator.cpp:1756
+ if (Alignment == CA_Preprocessor)
+(*I)->LevelOffset = 1;
} else {
krasimir wrote:
> This feels a bit awkward: we're adding code that implicitly assumes the exact
> style the preprocessor directives and comments around them are handled. Maybe
> if this could become part of the level itself, it would feel less awkward.
I agree that the "long distance coupling" is awkward. Perhaps the new
enumerator names make this a bit more palatable?
Comment at: lib/Format/TokenAnnotator.h:41
AnnotatedLine(const UnwrappedLine )
- : First(Line.Tokens.front().Tok), Level(Line.Level),
+ : First(Line.Tokens.front().Tok), Level(Line.Level), LevelOffset(0),
MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex),
krasimir wrote:
> Is there a way to not introduce `LevelOffset`, but have it part of `Level`?
`Level` is an abstract indentation level, while `LevelOffset` is "columns".
They have different units. Maybe it would be feasible to change the units of
"Level" to columns in order to merge these two variables, but doing so would
throw away information. It also seems like a much larger change. We could
create a composite type `class AnnotatedLevel { private: unsigned Level,
unsigned Offset public: }` but that seems
over-engineered. Any other ideas?
Repository:
rC Clang
https://reviews.llvm.org/D42036
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42036: [clang-format] Keep comments aligned to macros
mzeren-vmw updated this revision to Diff 130487.
mzeren-vmw added a comment.
Documented CommentAlignment enumerators. Documenting them suggested better
enumerator names.
Added tests for multi-line comments, block comments and trailing comments.
Repository:
rC Clang
https://reviews.llvm.org/D42036
Files:
lib/Format/TokenAnnotator.cpp
lib/Format/TokenAnnotator.h
lib/Format/UnwrappedLineFormatter.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2497,7 +2497,6 @@
"code();\n"
"#endif",
Style);
-
// Include guards must cover the entire file.
verifyFormat("code();\n"
"code();\n"
@@ -2593,21 +2592,97 @@
"code();\n"
"#endif",
Style));
- // FIXME: The comment indent corrector in TokenAnnotator gets thrown off by
- // preprocessor indentation.
- EXPECT_EQ("#if 1\n"
-" // comment\n"
-"# define A 0\n"
-"// comment\n"
-"# define B 0\n"
-"#endif",
-format("#if 1\n"
- "// comment\n"
- "# define A 0\n"
- " // comment\n"
- "# define B 0\n"
- "#endif",
- Style));
+ // Comments aligned to macros stay aligned. This test is incompatible with
+ // verifyFormat() because messUp() removes the alignment.
+ {
+const char *Expected = ""
+ "// Level 0 unaligned comment\n"
+ "// Level 0 unaligned continuation\n"
+ "#ifndef HEADER_H\n"
+ "// Level 0 aligned comment\n"
+ "// Level 0 aligned continuation\n"
+ "#define HEADER_H\n"
+ "#if 1\n"
+ " // aligned comment\n"
+ " // aligned continuation\n"
+ "# define A 0\n"
+ "// un-aligned comment\n"
+ "// un-aligned continuation\n"
+ "# define B 0\n"
+ "// Trailing aligned comment\n"
+ "#endif\n"
+ "#endif";
+const char *ToFormat = ""
+ " // Level 0 unaligned comment\n"
+ " // Level 0 unaligned continuation\n"
+ "#ifndef HEADER_H\n"
+ "// Level 0 aligned comment\n"
+ "// Level 0 aligned continuation\n"
+ "#define HEADER_H\n"
+ "#if 1\n"
+ " // aligned comment\n"
+ " // aligned continuation\n"
+ "# define A 0\n"
+ " // un-aligned comment\n"
+ " // un-aligned continuation\n"
+ "# define B 0\n"
+ " // Trailing aligned comment\n"
+ "#endif\n"
+ "#endif";
+EXPECT_EQ(Expected, format(ToFormat, Style));
+EXPECT_EQ(Expected, format(Expected, Style));
+ }
+ // Same as above, but block comments.
+ {
+const char *Expected = ""
+ "/*\n"
+ " * Level 0 unaligned block comment\n"
+ " */\n"
+ "#ifndef HEADER_H\n"
+ "/*\n"
+ " * Level 0 aligned comment\n"
+ " */\n"
+ "#define HEADER_H\n"
+ "#if 1\n"
+ " /*\n"
+ "* Aligned comment\n"
+ "*/\n"
+ "# define A 0\n"
+ "/*\n"
+ " * Unaligned comment\n"
+ " */\n"
+ "# define B 0\n"
+ "/*\n"
+ " * Trailing aligned comment\n"
+ " */\n"
+ "#endif\n"
+ "#endif";
+const char *ToFormat = ""
+ " /*\n"
+ " * Level 0 unaligned block comment\n"
+ " */\n"
+ "#ifndef HEADER_H\n"
+ "/*\n"
+ " * Level 0 aligned comment\n"
+ " */\n"
+ "#define HEADER_H\n"
+ "#if 1\n"
+ " /*\n"
+
[PATCH] D42036: [clang-format] Keep comments aligned to macros
krasimir added inline comments.
Comment at: lib/Format/TokenAnnotator.cpp:1710
+enum CommentAlignment { CA_None, CA_Code, CA_Preprocessor };
+
Please comment these.
Comment at: lib/Format/TokenAnnotator.cpp:1756
+ if (Alignment == CA_Preprocessor)
+(*I)->LevelOffset = 1;
} else {
This feels a bit awkward: we're adding code that implicitly assumes the exact
style the preprocessor directives and comments around them are handled. Maybe
if this could become part of the level itself, it would feel less awkward.
Comment at: lib/Format/TokenAnnotator.h:41
AnnotatedLine(const UnwrappedLine )
- : First(Line.Tokens.front().Tok), Level(Line.Level),
+ : First(Line.Tokens.front().Tok), Level(Line.Level), LevelOffset(0),
MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex),
Is there a way to not introduce `LevelOffset`, but have it part of `Level`?
Comment at: unittests/Format/FormatTest.cpp:2619
+ "#endif\n"
+ "#endif";
+EXPECT_EQ(Expected, format(ToFormat, Style));
I would like to see test including multiline `//`-comment sections before,
inside and after preprocessor directives as well as `/**/`-style comments.
Repository:
rC Clang
https://reviews.llvm.org/D42036
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42036: [clang-format] Keep comments aligned to macros
klimek added a comment. Just from a formatting point of view, why not: //. Comment #.define X Repository: rC Clang https://reviews.llvm.org/D42036 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42036: [clang-format] Keep comments aligned to macros
mzeren-vmw created this revision.
mzeren-vmw added reviewers: euhlmann, krasimir, klimek.
Herald added a subscriber: cfe-commits.
r312125, which introduced preprocessor indentation, shipped with a known
issue where "indentation of comments immediately before indented
preprocessor lines is toggled on each run". For example these two forms
toggle:
#ifndef HEADER_H
#define HEADER_H
#if 1
// comment
# define A 0
#endif
#endif
#ifndef HEADER_H
#define HEADER_H
#if 1
// comment
# define A 0
#endif
#endif
This happens because we check vertical alignment against the "#" yet
indent to the level of the "define". This patch resolves this issue by
checking vertical alignment against the "define", and by tracking a
"LevelOffset" (0 or 1) in each AnnotatedLine to account for the
off-by-one indentation of preprocessor lines.
Repository:
rC Clang
https://reviews.llvm.org/D42036
Files:
lib/Format/TokenAnnotator.cpp
lib/Format/TokenAnnotator.h
lib/Format/UnwrappedLineFormatter.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -2497,7 +2497,6 @@
"code();\n"
"#endif",
Style);
-
// Include guards must cover the entire file.
verifyFormat("code();\n"
"code();\n"
@@ -2593,21 +2592,34 @@
"code();\n"
"#endif",
Style));
- // FIXME: The comment indent corrector in TokenAnnotator gets thrown off by
- // preprocessor indentation.
- EXPECT_EQ("#if 1\n"
-" // comment\n"
-"# define A 0\n"
-"// comment\n"
-"# define B 0\n"
-"#endif",
-format("#if 1\n"
- "// comment\n"
- "# define A 0\n"
- " // comment\n"
- "# define B 0\n"
- "#endif",
- Style));
+ // Comments aligned to macros stay aligned. This test is incompatible with
+ // verifyFormat() because messUp() removes the alignment.
+ {
+const char *Expected = "// Level 0 unaligned comment\n"
+ "#ifndef HEADER_H\n"
+ "// Level 0 aligned comment\n"
+ "#define HEADER_H\n"
+ "#if 1\n"
+ " // aligned comment\n"
+ "# define A 0\n"
+ "// un-aligned comment\n"
+ "# define B 0\n"
+ "#endif\n"
+ "#endif";
+const char *ToFormat = " // Level 0 unaligned comment\n"
+ "#ifndef HEADER_H\n"
+ "// Level 0 aligned comment\n"
+ "#define HEADER_H\n"
+ "#if 1\n"
+ " // aligned comment\n"
+ "# define A 0\n"
+ " // un-aligned comment\n"
+ "# define B 0\n"
+ "#endif\n"
+ "#endif";
+EXPECT_EQ(Expected, format(ToFormat, Style));
+EXPECT_EQ(Expected, format(Expected, Style));
+ }
// Test with tabs.
Style.UseTab = FormatStyle::UT_Always;
Style.IndentWidth = 8;
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -52,6 +52,7 @@
/// next.
void nextLine(const AnnotatedLine ) {
Offset = getIndentOffset(*Line.First);
+Offset += Line.LevelOffset;
// Update the indent level cache size so that we can rely on it
// having the right size in adjustToUnmodifiedline.
while (IndentForLevel.size() <= Line.Level)
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -38,7 +38,7 @@
class AnnotatedLine {
public:
AnnotatedLine(const UnwrappedLine )
- : First(Line.Tokens.front().Tok), Level(Line.Level),
+ : First(Line.Tokens.front().Tok), Level(Line.Level), LevelOffset(0),
MatchingOpeningBlockLineIndex(Line.MatchingOpeningBlockLineIndex),
InPPDirective(Line.InPPDirective),
MustBeDeclaration(Line.MustBeDeclaration), MightBeFunctionDecl(false),
@@ -111,6 +111,12 @@
LineType Type;
unsigned Level;
+
+ /// Adjustment to Level based indent. When comments are aligned to the next
+ /// preprocessor line they must use the same offset as the directive,
+ /// typically 1 due to the leading #.
+ unsigned LevelOffset;
+
size_t MatchingOpeningBlockLineIndex;
bool InPPDirective;
bool MustBeDeclaration;
Index:
