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

Regression in clang-format-17 #62055

Closed
prj- opened this issue Apr 11, 2023 · 4 comments
Closed

Regression in clang-format-17 #62055

prj- opened this issue Apr 11, 2023 · 4 comments

Comments

@prj-
Copy link

prj- commented Apr 11, 2023

$ cat source.h 
template <bool other_is_const_it, util::enable_if_t<is_const_it && !other_is_const_it> * = nullptr>
table_iterator(const table_iterator<other_is_const_it> &other) noexcept : table_iterator(other.map_, other.it_)
{
}
$ clang-format --style=file:clang-format.txt source.h > 16.h
$ clang-format --version
clang-format version 16.0.1
$ clang-format-17 --style=file:clang-format.txt source.h > 17.h
$ clang-format-17 --version
Debian clang-format version 17.0.0 (++20230404111547+e45523cd218f-1~exp1~20230404111707.1206)
$ diff -Naur 16.h 17.h
--- 16.h	2023-04-11 07:20:48
+++ 17.h	2023-04-11 07:21:17
@@ -1,4 +1,4 @@
-template <bool other_is_const_it, util::enable_if_t<is_const_it && !other_is_const_it> * = nullptr>
+template <bool other_is_const_it, util::enable_if_t<is_const_it &&!other_is_const_it> * = nullptr>
 table_iterator(const table_iterator<other_is_const_it> &other) noexcept : table_iterator(other.map_, other.it_)
 {
 }

clang-format.txt is attached.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 11, 2023

@llvm/issue-subscribers-clang-format

@rymiel
Copy link
Member

rymiel commented Apr 11, 2023

Bisected to bb4f6c4, which is my commit, sorry

@rymiel rymiel self-assigned this Apr 11, 2023
@rymiel
Copy link
Member

rymiel commented Apr 11, 2023

It appears this is symptoms of a bigger bug. When I made that patch, I assumed that modifyContext.AssignmentStartsExpression would only touch the right side of the equals sign, since that's what the "starts" implies. Turns out it also affects the left side's formatting.

For example:

foo<a &&b>::x = 0;

I am 80% sure this could be valid C++ syntax in some cryptic scenario (a class with a NTTP, and an assignment to a static member variable). The && on the left hand side becomes a PointerOrReference due to AssignmentStartsExpression polluting it.

It also just so happens that this somewhat cryptic pattern is actually not that cryptic when dealing with SFINAE magic.

EDIT:

You don't even need cryptic syntax. Going by the original bug's syntax, even this is enough:

foo<a &&b> *c = nullptr;

EDIT2:

Nevermind, this has nothing to do with contexts, there just a strange correcting term that is run right after AssignmentStartsExpression that causes this.

@rymiel
Copy link
Member

rymiel commented Apr 11, 2023

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

@rymiel rymiel added the awaiting-review Has pending Phabricator review label Apr 11, 2023
@rymiel rymiel closed this as completed in 5dc94b3 Apr 11, 2023
@github-actions github-actions bot removed the awaiting-review Has pending Phabricator review label Apr 11, 2023
flemairen6 pushed a commit to Xilinx/llvm-project that referenced this issue May 10, 2023
After clang-format has determined that an equals sign starts an
expression, it will also go backwards and modify any star/amp/ampamp
binary operators on the left side of the assignment to be
pointers/references instead.

There already exists logic to skip over contents of parentheses and
square brackets, but this patch also expands that logic to apply to
angle brackets. This is so that binary operators inside of template
arguments would not be touched, primary arguments to non-type template
parameters.

Fixes llvm#62055

Reviewed By: owenpan, MyDeveloperDay, HazardyKnusperkeks

Differential Revision: https://reviews.llvm.org/D148024
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

4 participants