Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AlignConsecutiveDeclarations fails with attibutes #39764

Open
llvmbot opened this issue Jan 23, 2019 · 3 comments
Open

AlignConsecutiveDeclarations fails with attibutes #39764

llvmbot opened this issue Jan 23, 2019 · 3 comments
Labels
bugzilla Issues migrated from bugzilla clang-format confirmed Verified by a second party

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 23, 2019 •
edited by mkurdej

Bugzilla Link 40418
Version 7.0
OS All
Reporter LLVM Bugzilla Contributor
CC @mydeveloperday

Extended Description

clang-format produces:

void f(int  extremlylongparameternamexxxxxxxx1,
       long extremlylongparameternamexxxxxxxx2,
       int[[]] extremlylongparameternamexxxxxxxx3,
       int __attribute__(()) extremlylongparameternamexxxxxxxx4) {
  int           a;
  unsigned long b;
  int[[]] c;
  int __attribute__(()) d;
}

Given configuration:

AlignConsecutiveDeclarations: true

It fails to align both the parameters and variables declarations with attributes. Other lines work fine. But those with attributes are ignored.
@mydeveloperday
Copy link
Contributor

mydeveloperday commented Mar 13, 2019 •
edited by mkurdej

clang-format doesn't see the ( and [ of the attributes as being part of what can determine the space between the type and the name

void f(int extremlylongparameternamexxxxxxxx1, long extremlylongparameternamexxxxxxxx2,
       int[[]] extremlylongparameternamexxxxxxxx3,
       int __attribute__(()) extremlylongparameternamexxxxxxxx4) {
  int           a;
  unsigned long b;
  int __attribute__(()) d;
  int[[]] c;
}

void f(int extremlylongparameternamexxxxxxxx1, long extremlylongparameternamexxxxxxxx2,
       int____ extremlylongparameternamexxxxxxxx3,
       int __attribute______ extremlylongparameternamexxxxxxxx4) {
  int           a;
  unsigned long b;
  int __attribute______ d;
  int____               c;
}

void f(int extremlylongparameternamexxxxxxxx1, long extremlylongparameternamexxxxxxxx2,
       int [[clang::nodiscard]] extremlylongparameternamexxxxxxxx3,
       int __attribute__((clang::nodiscard)) extremlylongparameternamexxxxxxxx4) {
  int           a;
  unsigned long b;
  int __attribute__((clang::nodiscard)) d;
  int [[clang::nodiscard]] c;
}

When we run this with the --debug option if we look at this line

int __attribute______ d;

it's represented as

AnnotatedTokens(L=1):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x1afa8e48470 Text='int'
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=220 Name=identifier L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x1afa8e49f70 Text='__attribute______'
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=130 Name=identifier L=23 PPK=2 FakeLParens= FakeRParens=0 II=0x1afa8e49ed0 Text='d'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----

but this line:

int __attribute__((clang::nodiscard)) d;

is more complex,

AnnotatedTokens(L=1):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= FakeRParens=0 II=0x1afa8e48470 Text='int'
 M=0 C=1 T=Unknown S=1 B=0 BK=0 P=23 Name=__attribute L=17 PPK=2 FakeLParens= FakeRParens=0 II=0x1afa8e46ee8 Text='__attribute__'
 M=0 C=0 T=AttributeParen S=0 B=0 BK=0 P=23 Name=l_paren L=18 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=59 Name=l_paren L=19 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=79 Name=identifier L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x1afa8e49fa0 Text='clang'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=63 Name=coloncolon L=26 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='::'
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=560 Name=identifier L=35 PPK=2 FakeLParens= FakeRParens=0 II=0x1afa8e49fd8 Text='nodiscard'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=63 Name=r_paren L=36 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=AttributeParen S=0 B=0 BK=0 P=43 Name=r_paren L=37 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=Unknown S=1 B=0 BK=0 P=23 Name=identifier L=39 PPK=2 FakeLParens= FakeRParens=0 II=0x1afa8e49ed0 Text='d'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=semi L=40 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'
----

In the code, I suspect it needs to be able to handle other token types(l_parent,identifier,coloncolon,r_paren)

void WhitespaceManager::alignConsecutiveDeclarations() {
  if (!Style.AlignConsecutiveDeclarations)
    return;

  // FIXME: Currently we don't handle properly the PointerAlignment: Right
  // The * and & are not aligned and are left dangling. Something has to be done
  // about it, but it raises the question of alignment of code like:
  //   const char* const* v1;
  //   float const* v2;
  //   SomeVeryLongType const& v3;
  AlignTokens(Style,
              [](Change const &C) {
                // tok::kw_operator is necessary for aligning operator overload
                // definitions.
                return C.Tok->is(TT_StartOfName) ||
                       C.Tok->is(TT_FunctionDeclarationName) ||
                       C.Tok->is(tok::kw_operator);
              },
              Changes, /*StartAt=*/0);
}

One way is to look for specific sequences

        return C.Tok->is(TT_StartOfName) ||
               C.Tok->is(TT_FunctionDeclarationName) ||
               C.Tok->startsSequence(tok::l_paren, tok::l_paren, tok::identifier,
                                  tok::coloncolon, tok::identifier,
                                  tok::r_paren, tok::r_paren) ||
               C.Tok->startsSequence(tok::l_square, tok::l_square, tok::identifier,
                                  tok::coloncolon, tok::identifier,
                                  tok::r_square, tok::r_square) ||
               C.Tok->is(tok::kw_operator);

Which handles some cases but not your empty attribute example of [[]] and __attribute(())

After adding these 2 C.Tok->startsSequence clauses, I get the following:

void f(int              extremlylongparameternamexxxxxxxx1,
       long             extremlylongparameternamexxxxxxxx2,
       int              [[a::b]] extremlylongparameternamexxxxxxxx3,
       int __attribute__((a::b)) extremlylongparameternamexxxxxxxx4) {
  int              a;
  unsigned long    b;
  int __attribute__((a::b)) d;
  int              [[a::b]] c;
  int __attribute__((a::b)) e;
  int __attribute__((depreciated)) f;
  int __attribute__((aligned(16))) g;
}

but the following with your example

void f(int  extremlylongparameternamexxxxxxxx1,
       long extremlylongparameternamexxxxxxxx2,
       int[[]] extremlylongparameternamexxxxxxxx3,
       int __attribute__(()) extremlylongparameternamexxxxxxxx4) {
  int           a;
  unsigned long b;
  int __attribute__(()) d;
  int[[]] c;
  int __attribute__(()) e;
  int __attribute__((depreciated)) f;
  int __attribute__((aligned(16))) g;
}

Although they too can be handled by adding:

return C.Tok->is(TT_StartOfName) ||
               C.Tok->is(TT_FunctionDeclarationName) ||
               C.Tok->startsSequence(tok::l_paren, tok::l_paren, tok::identifier,
                                  tok::coloncolon, tok::identifier,
                                  tok::r_paren, tok::r_paren) ||
               C.Tok->startsSequence(tok::l_square, tok::l_square, tok::identifier,
                                  tok::coloncolon, tok::identifier,
                                  tok::r_square, tok::r_square) ||
               C.Tok->startsSequence(tok::l_paren, tok::l_paren,
                                  tok::r_paren, tok::r_paren) ||
               C.Tok->startsSequence(tok::l_square, tok::l_square,
                                  tok::r_square, tok::r_square) ||
               C.Tok->is(tok::kw_operator);

void f(int              extremlylongparameternamexxxxxxxx1,
       long             extremlylongparameternamexxxxxxxx2,
       int              [[]] extremlylongparameternamexxxxxxxx3,
       int __attribute__(()) extremlylongparameternamexxxxxxxx4) {
  int              a;
  unsigned long    b;
  int __attribute__(()) d;
  int              [[]] c;
  int __attribute__(()) e;
  int __attribute__((depreciated)) f;
  int __attribute__((aligned(16))) g;
}

Before submitting a patch, I'd like to understand if the your empty example has a real world usecase?

MyDeveloperDay

@llvmbot
Copy link
Collaborator Author

llvmbot commented Mar 13, 2019

The empty argument example was just a result of simplifying the real world use case. Our code-base never has empty attribute lists.

@mydeveloperday
Copy link
Contributor

WIP https://reviews.llvm.org/D59332

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@llvmbot llvmbot added the confirmed Verified by a second party label Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla clang-format confirmed Verified by a second party
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mydeveloperday @llvmbot