-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[clang-format] Formatting regression for member function pointers using aligned assignment and declaration #68079
[clang-format] Formatting regression for member function pointers using aligned assignment and declaration #68079
Comments
The root of the problem here is that clang-format uses the FunctionTypeLParen to infer a function pointer from something that looks like a function declaration. So the assignment of a function pointer to a variable is triggering logic for both AlignConsecutiveAssignments and AlignConsecutiveDeclarations, and their interaction is resulting in this regression. Disabling alignment of assignments, there is also some difference between these versions of clang-format despite that my commit only modifies the AlignConsecutiveDeclarations part: newer version:
older version:
I'm not sure, but I think the I can try to work on a fix to better isolate declarations and assignments. The documentation is a bit confusing about what the differences are between them. https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignconsecutivedeclarations |
In looking at the regression, it appears to be that the I just did a quick test of
and this gives:
This appears to fix the regression., but it breaks other tests. I'll look into finding a better fix but not sure when I can devote solid time to it. |
I will revert a84e0b4 until we can come up with a good solution. We also must decide whether aligning on the |
I have an OK solution. It may need a little reworking. I'll submit a rev shortly.
Where does it say to align on the name? Actually, the documentation for AlignConsecutiveDeclarations appears to be mixed with AlignConsecutiveMacros. |
Thanks! Nevertheless, we should, IMO, still revert the offending patch for 17.x as your new solution, if merged, would be too late to be backported. What do you think, @mydeveloperday, @HazardyKnusperkeks, and @rymiel?
From the documentation I linked above: It seems more consistent to align the function pointer names just like any pointer/reference names. For example:
|
17.0.3 is "just" released, so for that everything is too late. No need for a revert. My question to the tests of #69340 is: Do we want to align the "declarations" after the equal? That aren't actually declarations, or am I wrong? |
You are right. In fact, I was going to make a quick fix by excluding |
That's a very good point. It can be reduced down similar to this example to whether it is preferred to have:
or to have
The OP issue does not have this problem. I think it is outside of the current documentation, but the second option is consistent with the notion that a function pointer is not a declaration. Are there any declarations that come after the equal operator? If not, then the fix is pretty simple:
|
Not that I know of. Only inside a lambda, but I don't think that we would hit that.
Would be nice if there is another solution with a O(1) check, instead of that loop. But I'll accept it. |
I pushed this approach to the PR and I think it's ready to review and hopefully merge. I agree but I didn't think of any. The required information is not available at the line level to filter out lines once they've been partially aligned. |
IMO, because a84e0b4 was kind of a new feature (aligning function pointers with other declaration names) and caused a regression in 17.x, we should revert it in 17.0.4 (if it's ok by @tru). We can then add it in 18.x after reaching a consensus on whether to align the left parenthesis or the name. We definitely need input from @mydeveloperday on this. |
I'm reverting a84e0b4 for 17.x while we are deciding on how to move forward with aligning function pointers. |
/cherry-pick 7bc1031 |
/branch llvm/llvm-project-release-prs/issue68079 |
/pull-request llvm/llvm-project-release-prs#747 |
Aligning on the function pointer name is non-trivial. There can be complex conditions such as functions that return function pointers, in which case you end up with several nesting levels of parentheses and stars. I'm not confident that a simple solution exists that can manipulate the alignment shift in the whitespace. |
Now I think aligning the left parenthesis is the correct thing to do:
We just need an option to turn it on/off. |
Function pointers are detected as a type of declaration using FunctionTypeLParen. They are aligned based on rules for AlignConsecutiveDeclarations. When a function pointer is on the right-hand side of an assignment, the alignment of the function pointer can result in excessive whitespace padding due to the ordering of alignment, as the alignment processes a line from left-to-right and first aligns the declarations before and after the assignment operator, and then aligns the assignment operator. Injection of whitespace by alignment of declarations after the equal sign followed by alignment of the equal sign results in the excessive whitespace. Fixes llvm#68079.
…9340) Function pointers are detected as a type of declaration using FunctionTypeLParen. They are aligned based on rules for AlignConsecutiveDeclarations. When a function pointer is on the right-hand side of an assignment, the alignment of the function pointer can result in excessive whitespace padding due to the ordering of alignment, as the alignment processes a line from left-to-right and first aligns the declarations before and after the assignment operator, and then aligns the assignment operator. Injection of whitespace by alignment of declarations after the equal sign followed by alignment of the equal sign results in the excessive whitespace. Fixes #68079.
…vm#69340) Function pointers are detected as a type of declaration using FunctionTypeLParen. They are aligned based on rules for AlignConsecutiveDeclarations. When a function pointer is on the right-hand side of an assignment, the alignment of the function pointer can result in excessive whitespace padding due to the ordering of alignment, as the alignment processes a line from left-to-right and first aligns the declarations before and after the assignment operator, and then aligns the assignment operator. Injection of whitespace by alignment of declarations after the equal sign followed by alignment of the equal sign results in the excessive whitespace. Fixes llvm#68079.
I've recently upgraded my tools from LLVM-16 to LLVM-17 and notices the following regression with clang-format.
.clang-format
test.cpp
results in the following formatting:
The text was updated successfully, but these errors were encountered: