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.

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

[format] Complex requires clauses get formatted wildly #63251

Closed
danakj opened this issue Jun 11, 2023 · 5 comments
Closed

[format] Complex requires clauses get formatted wildly #63251

danakj opened this issue Jun 11, 2023 · 5 comments
Assignees

Comments

@danakj
Copy link
Contributor

danakj commented Jun 11, 2023

Clang-format produces this:

#include <concepts>

template <class T, class E>
void Result()
    requires(!((std::is_void_v<T> ||
                std::is_trivially_destructible_v<
                    T>)&&std::is_trivially_destructible_v<E>) &&
             !std::is_void_v<T>)
{}

Instead of something like this:

#include <concepts>

template <class T, class E>
void Result()
    requires(!((std::is_void_v<T> || std::is_trivially_destructible_v<T>) &&
               std::is_trivially_destructible_v<E>) &&
             !std::is_void_v<T>)
{}
  • It wraps at the template parameter of is_trivially_destructible_v.
  • It puts no spaces around the && after it.

See it in action: https://godbolt.org/z/br3hddTbf

@danakj
Copy link
Contributor Author

danakj commented Jun 11, 2023

Here's a smaller one that goes wrong already, the && loses its spaces around it.

#include <concepts>

template <class T, class E>
void Result()
    requires(!((std::is_void_v<T> || std::is_void_v<T>)&&::std::is_void_v<E>))
{}

Instead of

#include <concepts>

template <class T, class E>
void Result()
    requires(!((std::is_void_v<T> || std::is_void_v<T>) && ::std::is_void_v<E>))
{}

@llvmbot
Copy link
Collaborator

llvmbot commented Jun 12, 2023

@llvm/issue-subscribers-clang-format

@rymiel
Copy link
Member

rymiel commented Jun 23, 2023

Further reduction of the second case:

void main()
  requires(((A<T>)&&B))
{}

Pertinent debug information:

 M=1 C=1 T=RequiresClause S=1 F=0 B=0 BK=0 P=23 Name=requires L=91 PPK=2 FakeLParens= FakeRParens=0 II=0x55c6c233a178 Text='requires'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=92 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=140 Name=l_paren L=93 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=160 Name=l_paren L=94 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=180 Name=identifier L=95 PPK=2 FakeLParens= FakeRParens=0 II=0x55c6c236d5b0 Text='A'
 M=0 C=0 T=TemplateOpener S=0 F=0 B=0 BK=0 P=90 Name=less L=96 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='<'
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=420 Name=identifier L=97 PPK=2 FakeLParens= FakeRParens=0 II=0x55c6c236d5e0 Text='T'
 M=0 C=0 T=TemplateCloser S=0 F=0 B=0 BK=0 P=330 Name=greater L=98 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='>'
 M=0 C=0 T=CastRParen S=0 F=0 B=0 BK=0 P=90 Name=r_paren L=99 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=UnaryOperator S=0 F=0 B=0 BK=0 P=160 Name=ampamp L=101 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='&&'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=120 Name=identifier L=102 PPK=2 FakeLParens= FakeRParens=0 II=0x55c6c236d610 Text='B'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=63 Name=r_paren L=103 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=104 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'

The && is somehow becoming an unary operator, and the parens before it a cast. Note that the redundant outer parens are required for this bug to manifest. (They aren't redundant if you add the ! in front, though)

@rymiel rymiel self-assigned this Jun 23, 2023
@rymiel rymiel added the awaiting-review Has pending Phabricator review label Jun 23, 2023
@rymiel
Copy link
Member

rymiel commented Jun 23, 2023

Patch: https://reviews.llvm.org/D153641

@rymiel rymiel closed this as completed in 15e14f1 Jun 26, 2023
@github-actions github-actions bot removed the awaiting-review Has pending Phabricator review label Jun 26, 2023
@Cons-Cat
Copy link

Patch: https://reviews.llvm.org/D153641

Thank you so much! This issue has annoyed me for awhile

Chenyang-L pushed a commit to intel/llvm that referenced this issue Jul 11, 2023
When parsing a requires clause, the UnwrappedLineParser would delegate to
parseParens with an AmpAmpTokenType set to BinaryOperator. However,
parseParens would not carry this over into any nested parens, meaning it
could assign a different token type to an && in a requires clause.

This patch makes sure parseParens inherits its parameter when performing
a recursive call.

Fixes llvm/llvm-project#63251

Reviewed By: HazardyKnusperkeks, owenpan, MyDeveloperDay

Differential Revision: https://reviews.llvm.org/D153641
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants