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

clang-format: IntegerLiteralSeparator formatted incorrectly with s literal suffix #62679

Closed
vogelsgesang opened this issue May 12, 2023 · 6 comments · Fixed by llvm/llvm-project-release-prs#448
Closed

clang-format: IntegerLiteralSeparator formatted incorrectly with s literal suffix #62679

vogelsgesang opened this issue May 12, 2023 · 6 comments · Fixed by llvm/llvm-project-release-prs#448
Assignees
@owenca
Labels
clang-format release:backport
Milestone
LLVM 16.0.X Release

Comments

@vogelsgesang
Copy link
Member

vogelsgesang commented May 12, 2023 •
edited by VoltrexKeyva

WIth the configuration

IntegerLiteralSeparator:
  Binary: 8
  Decimal: 3
  DecimalMinDigits: 5
  Hex: 0

Clang-Format 16.0.3 proposes the following change:

-   RateLimit limit{2, 1000s};
+   RateLimit limit{2, 10'00s};

There are two issues:

  1. it seems to consider the s suffix as a digit, and hence place the tick in the wrong place
  2. it should not format this number at all, thanks to DecimalMinDigits
@llvmbot
Copy link
Collaborator

llvmbot commented May 12, 2023

@llvm/issue-subscribers-clang-format

@vogelsgesang
Copy link
Member Author

this looks very similar to #61676, but it seems the fix did not cover this case here

@vogelsgesang vogelsgesang changed the title IntegerLiteralSeparator not correct with s literal suffix clang-format: IntegerLiteralSeparator formatted incorrectly with s literal suffix May 12, 2023
@owenca owenca self-assigned this May 14, 2023
@owenca
Copy link
Contributor

owenca commented May 15, 2023

https://reviews.llvm.org/D150539

@owenca owenca added the awaiting-review Has pending Phabricator review label May 15, 2023
@owenca owenca closed this as completed in a72b064 May 16, 2023
@github-actions github-actions bot removed the awaiting-review Has pending Phabricator review label May 16, 2023
@owenca
Copy link
Contributor

owenca commented May 16, 2023

/cherry-pick a72b064

@owenca owenca reopened this May 16, 2023
@owenca owenca added this to the LLVM 16.0.X Release milestone May 16, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented May 16, 2023

/branch llvm/llvm-project-release-prs/issue62679

@llvmbot llvmbot mentioned this issue May 16, 2023
Merged
@llvmbot
Copy link
Collaborator

llvmbot commented May 16, 2023

/pull-request llvm/llvm-project-release-prs#448

tstellar pushed a commit that referenced this issue May 30, 2023
@owenca @tstellar
[clang-format] Handle <chrono> ud suffixes in IntegerLiteralSeparator
35bd94a 
Fixes #62679.

Differential Revision: https://reviews.llvm.org/D150539

(cherry picked from commit a72b064)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@owenca owenca

Labels
clang-format release:backport
Projects
LLVM Release Status
Archived in project
Milestone
LLVM 16.0.X Release
Development

Successfully merging a pull request may close this issue.

PR for llvm/llvm-project#62679 llvm/llvm-project-release-prs
3 participants
@owenca @vogelsgesang @llvmbot