On Mon, May 6, 2013 at 1:27 AM, Daniel Jasper <[email protected]> wrote:
> Author: djasper
> Date: Mon May 6 03:27:33 2013
> New Revision: 181187
>
> URL: http://llvm.org/viewvc/llvm-project?rev=181187&view=rev
> Log:
> Change indentation when breaking after a type.
>
> clang-format did not indent any declarations/definitions when breaking
> after the type. With this change, it indents for all declarations but
> does not indent for function definitions, i.e.:
>
> Before:
> const SomeLongTypeName&
> some_long_variable_name;
> typedef SomeLongTypeName
> SomeLongTypeAlias;
> const SomeLongReturnType*
> SomeLongFunctionName();
> const SomeLongReturnType*
> SomeLongFunctionName() { ... }
>
> After:
> const SomeLongTypeName&
> some_long_variable_name;
> typedef SomeLongTypeName
> SomeLongTypeAlias;
> const SomeLongReturnType*
> SomeLongFunctionName();
No judgment on how good/bad this is, but this particular case seems
inconsistent with LLVM's defacto standard style. Function declarations
still don't tend to have the indentation for the function name. (I'm
looking at llvm/include/llvm/DIBuilder.h at the moment)
> const SomeLongReturnType*
> SomeLongFunctionName() { ... }
>
> While it might seem inconsistent to indent function declarations, but
> not definitions, there are two reasons for that:
> - Function declarations are very similar to declarations of function
> type variables, so there is another side to consistency to consider.
> - There can be many function declarations on subsequent lines and not
> indenting can make them harder to identify. Function definitions
> are already separated by their body and not indenting
> makes the function name slighly easier to find.
>
> Modified:
> cfe/trunk/lib/Format/Format.cpp
> cfe/trunk/lib/Format/TokenAnnotator.cpp
> cfe/trunk/lib/Format/TokenAnnotator.h
> cfe/trunk/unittests/Format/FormatTest.cpp
>
> Modified: cfe/trunk/lib/Format/Format.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=181187&r1=181186&r2=181187&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Format/Format.cpp (original)
> +++ cfe/trunk/lib/Format/Format.cpp Mon May 6 03:27:33 2013
> @@ -359,7 +359,8 @@ private:
> State.Stack.back().VariablePos != 0) {
> State.Column = State.Stack.back().VariablePos;
> } else if (Previous.ClosesTemplateDeclaration ||
> - (Current.Type == TT_StartOfName && State.ParenLevel == 0)) {
> + (Current.Type == TT_StartOfName && State.ParenLevel == 0 &&
> + Line.StartsDefinition)) {
> State.Column = State.Stack.back().Indent;
> } else if (Current.Type == TT_ObjCSelectorName) {
> if (State.Stack.back().ColonPos > Current.FormatTok.TokenLength) {
>
> Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=181187&r1=181186&r2=181187&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
> +++ cfe/trunk/lib/Format/TokenAnnotator.cpp Mon May 6 03:27:33 2013
> @@ -245,24 +245,25 @@ private:
> }
>
> bool parseBrace() {
> - // Lines are fine to end with '{'.
> - if (CurrentToken == NULL)
> - return true;
> - ScopedContextCreator ContextCreator(*this, tok::l_brace, 1);
> - AnnotatedToken *Left = CurrentToken->Parent;
> - while (CurrentToken != NULL) {
> - if (CurrentToken->is(tok::r_brace)) {
> - Left->MatchingParen = CurrentToken;
> - CurrentToken->MatchingParen = Left;
> - next();
> - return true;
> + if (CurrentToken != NULL) {
> + ScopedContextCreator ContextCreator(*this, tok::l_brace, 1);
> + AnnotatedToken *Left = CurrentToken->Parent;
> + while (CurrentToken != NULL) {
> + if (CurrentToken->is(tok::r_brace)) {
> + Left->MatchingParen = CurrentToken;
> + CurrentToken->MatchingParen = Left;
> + next();
> + return true;
> + }
> + if (CurrentToken->isOneOf(tok::r_paren, tok::r_square))
> + return false;
> + updateParameterCount(Left, CurrentToken);
> + if (!consumeToken())
> + return false;
> }
> - if (CurrentToken->isOneOf(tok::r_paren, tok::r_square))
> - return false;
> - updateParameterCount(Left, CurrentToken);
> - if (!consumeToken())
> - return false;
> }
> + // No closing "}" found, this probably starts a definition.
> + Line.StartsDefinition = true;
> return true;
> }
>
>
> Modified: cfe/trunk/lib/Format/TokenAnnotator.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.h?rev=181187&r1=181186&r2=181187&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Format/TokenAnnotator.h (original)
> +++ cfe/trunk/lib/Format/TokenAnnotator.h Mon May 6 03:27:33 2013
> @@ -209,8 +209,8 @@ public:
> AnnotatedLine(const UnwrappedLine &Line)
> : First(Line.Tokens.front()), Level(Line.Level),
> InPPDirective(Line.InPPDirective),
> - MustBeDeclaration(Line.MustBeDeclaration),
> - MightBeFunctionDecl(false) {
> + MustBeDeclaration(Line.MustBeDeclaration),
> MightBeFunctionDecl(false),
> + StartsDefinition(false) {
> assert(!Line.Tokens.empty());
> AnnotatedToken *Current = &First;
> for (std::list<FormatToken>::const_iterator I = ++Line.Tokens.begin(),
> @@ -226,7 +226,8 @@ public:
> : First(Other.First), Type(Other.Type), Level(Other.Level),
> InPPDirective(Other.InPPDirective),
> MustBeDeclaration(Other.MustBeDeclaration),
> - MightBeFunctionDecl(Other.MightBeFunctionDecl) {
> + MightBeFunctionDecl(Other.MightBeFunctionDecl),
> + StartsDefinition(Other.StartsDefinition) {
> Last = &First;
> while (!Last->Children.empty()) {
> Last->Children[0].Parent = Last;
> @@ -242,6 +243,7 @@ public:
> bool InPPDirective;
> bool MustBeDeclaration;
> bool MightBeFunctionDecl;
> + bool StartsDefinition;
> };
>
> inline prec::Level getPrecedence(const AnnotatedToken &Tok) {
>
> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=181187&r1=181186&r2=181187&view=diff
> ==============================================================================
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Mon May 6 03:27:33 2013
> @@ -1520,7 +1520,7 @@ TEST_F(FormatTest, MixingPreprocessorDir
> TEST_F(FormatTest, LayoutStatementsAroundPreprocessorDirectives) {
> EXPECT_EQ("int\n"
> "#define A\n"
> - "a;",
> + " a;",
> format("int\n#define A\na;"));
> verifyFormat("functionCallTo(\n"
> " someOtherFunction(\n"
> @@ -1713,7 +1713,7 @@ TEST_F(FormatTest, MemoizationTests) {
> "CFRunLoopTimerCreate(CFAllocatorRef allocato, CFAbsoluteTime
> fireDate,\n"
> " CFTimeInterval interval, CFOptionFlags flags,\n"
> " CFIndex order, CFRunLoopTimerCallBack callout,\n"
> - " CFRunLoopTimerContext *context);");
> + " CFRunLoopTimerContext *context) {}");
>
> // Deep nesting somewhat works around our memoization.
> verifyFormat(
> @@ -1755,8 +1755,9 @@ TEST_F(FormatTest, BreaksFunctionDeclara
> " Cccccccccccccc
> cccccccccccccc);");
>
> // 2) break after return type.
> - verifyFormat("Aaaaaaaaaaaaaaaaaaaaaaaa\n"
> - "bbbbbbbbbbbbbb(Cccccccccccccc cccccccccccccccccccccccccc);");
> + verifyFormat(
> + "Aaaaaaaaaaaaaaaaaaaaaaaa\n"
> + " bbbbbbbbbbbbbb(Cccccccccccccc cccccccccccccccccccccccccc);");
>
> // 3) break after (.
> verifyFormat(
> @@ -1766,8 +1767,8 @@ TEST_F(FormatTest, BreaksFunctionDeclara
> // 4) break before after nested name specifiers.
> verifyFormat(
> "Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> - "SomeClasssssssssssssssssssssssssssssssssssssss::\n"
> - " bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb(Cccccccccccccc cccccccccc);");
> + " SomeClasssssssssssssssssssssssssssssssssssssss::\n"
> + " bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb(Cccccccccccccc
> cccccccccc);");
>
> // However, there are exceptions, if a sufficient amount of lines can be
> // saved.
> @@ -1780,9 +1781,9 @@ TEST_F(FormatTest, BreaksFunctionDeclara
> " Cccccccccccccc
> cccccccccc);");
> verifyFormat(
> "Aaaaaaaaaaaaaaaaaa\n"
> - "bbbbbbbbbbbbbb(Cccccccccccccc cccccccccc, Cccccccccccccc
> cccccccccc,\n"
> - " Cccccccccccccc cccccccccc, Cccccccccccccc
> cccccccccc,\n"
> - " Cccccccccccccc cccccccccc, Cccccccccccccc
> cccccccccc);");
> + " bbbbbbbbbbb(Cccccccccccccc cccccccccc, Cccccccccccccc
> cccccccccc,\n"
> + " Cccccccccccccc cccccccccc, Cccccccccccccc
> cccccccccc,\n"
> + " Cccccccccccccc cccccccccc, Cccccccccccccc
> cccccccccc);");
> verifyFormat(
> "Aaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbb(Cccccccccccccc
> cccccccccc,\n"
> " Cccccccccccccc
> cccccccccc,\n"
> @@ -1954,12 +1955,9 @@ TEST_F(FormatTest, DoesNotBreakTrailingA
> " aaaaaaaaaaaaaaaaaaaaaaaaa));");
> verifyFormat("bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> " __attribute__((unused));");
> -
> - // FIXME: This is bad indentation, but generally hard to distinguish from a
> - // function declaration.
> verifyFormat(
> "bool aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> - "GUARDED_BY(aaaaaaaaaaaa);");
> + " GUARDED_BY(aaaaaaaaaaaa);");
> }
>
> TEST_F(FormatTest, BreaksAccordingToOperatorPrecedence) {
> @@ -2121,8 +2119,8 @@ TEST_F(FormatTest, DeclarationsOfMultipl
> // FIXME: If multiple variables are defined, the "*" needs to move to the
> new
> // line. Also fix indent for breaking after the type, this looks bad.
> verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa *\n"
> - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaa = aaaaaaaaaaaaaaaaaaa,\n"
> - " *b = bbbbbbbbbbbbbbbbbbb;");
> + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaa = aaaaaaaaaaaaaaaaaaa,\n"
> + " *b = bbbbbbbbbbbbbbbbbbb;");
>
> // Not ideal, but pointer-with-type does not allow much here.
> verifyGoogleFormat(
> @@ -2707,6 +2705,19 @@ TEST_F(FormatTest, FormatsFunctionTypes)
> }
>
> TEST_F(FormatTest, BreaksLongDeclarations) {
> + verifyFormat("typedef LoooooooooooooooooooooooooooooooooooooooongType\n"
> + " AnotherNameForTheLongType;");
> + verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
> + " LoooooooooooooooooooooooooooooooooooooooongVariable;");
> + verifyFormat("LoooooooooooooooooooooooooooooooooooooooongReturnType\n"
> + "
> LoooooooooooooooooooooooooooooooongFunctionDeclaration();");
> + verifyFormat("LoooooooooooooooooooooooooooooooooooooooongReturnType\n"
> + "LooooooooooooooooooooooooooooooooooongFunctionDefinition()
> {}");
> +
> + // FIXME: Without the comment, this breaks after "(".
> + verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType // break\n"
> + "
> (*LoooooooooooooooooooooooooooongFunctionTypeVarialbe)();");
> +
> verifyFormat("int *someFunction(int LoooooooooooooooooooongParam1,\n"
> " int LoooooooooooooooooooongParam2) {}");
> verifyFormat(
> @@ -2722,7 +2733,7 @@ TEST_F(FormatTest, BreaksLongDeclaration
> " AnotherLongParameterName) {}");
> verifyFormat(
> "aaaaaaaaaaaaaaaa::aaaaaaaaaaaaaaaa<aaaaaaaaaaaaa, aaaaaaaaaaaa>\n"
> - "aaaaaaaaaaaaaaaaaaaaaaa;");
> + " aaaaaaaaaaaaaaaaaaaaaaa;");
>
> verifyGoogleFormat(
> "TypeSpecDecl* TypeSpecDecl::Create(ASTContext& C, DeclContext* DC,\n"
>
>
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
