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 option AfterPlacementOperator should be a boolean #78892

Closed
owenca opened this issue Jan 21, 2024 · 7 comments · Fixed by #79796
Closed

clang-format option AfterPlacementOperator should be a boolean #78892

owenca opened this issue Jan 21, 2024 · 7 comments · Fixed by #79796
Assignees
@owenca
Labels
clang-format release:backport
Milestone
LLVM 18.X Release

Comments

@owenca
Copy link
Contributor

owenca commented Jan 21, 2024

IMO, we don't need the Leave value for AfterPlacementOperator. @mydeveloperday @HazardyKnusperkeks @rymiel @omarahmed1111 what do you think?
@llvmbot
Copy link

llvmbot commented Jan 21, 2024

@llvm/issue-subscribers-clang-format

Author: Owen Pan (owenca)

IMO, we don't need the `Leave` value for `AfterPlacementOperator`. @mydeveloperday @HazardyKnusperkeks @rymiel @omarahmed1111 what do you think?

@HazardyKnusperkeks
Copy link
Contributor

One does never really need a Leave. But I someone even asked for a leave style to adapt the formatting options one by one. And from that perspective I'm in favor of Leave options, although I'll most likely never use one myself.

@owenca
Copy link
Contributor Author

owenca commented Jan 21, 2024

A Leave value is needed for some options (e.g. BreakAfterAttributes) but not options like AfterPlacementOperator IMO. Why would we want to support inconsistency in the formatted code irt whether to have a space between the new/delete keyword and the opening parenthesis?

Also, only AfterPlacementOperator is an enum in SpaceBeforeParensOptions. Everything else is a bool. So I think we should change AfterPlacementOperator to a boolean for simplicity and consistency.

@owenca owenca self-assigned this Jan 21, 2024
@HazardyKnusperkeks
Copy link
Contributor

If you want to convert, that should happen before the LLVM 18 release, after that I'd say we can't remove the Leave option anymore.

owenca added a commit to owenca/llvm-project that referenced this issue Jan 29, 2024
@owenca
[clang-format] Simplify the AfterPlacementOperator option
46f4a74 
Change AfterPlacementOperator to a boolean. Also add SBPO_None for never
inserting a space before a left parenthesis and deprecate SBPO_Never, which
meant never inserting a space except when after new/delete.

Fixes llvm#78892.
@owenca owenca mentioned this issue Jan 29, 2024
Merged
@owenca owenca added this to the LLVM 18.X Release milestone Jan 29, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Jan 29, 2024
owenca added a commit to owenca/llvm-project that referenced this issue Jan 29, 2024
@owenca
[clang-format] Simplify the AfterPlacementOperator option
d40f2ed 
Change AfterPlacementOperator to a boolean. Also add SBPO_None for never
inserting a space before a left parenthesis and deprecate SBPO_Never, which
meant never inserting a space except when after new/delete.

Fixes llvm#78892.
@tstellar tstellar moved this from Needs Triage to Needs Fix in LLVM Release Status Jan 29, 2024
owenca added a commit that referenced this issue Feb 1, 2024
@owenca
[clang-format] Simplify the AfterPlacementOperator option (#79796)
908fd09 
Change AfterPlacementOperator to a boolean and deprecate SBPO_Never,
which meant never inserting a space except when after new/delete.

Fixes #78892.
@owenca
Copy link
Contributor Author

owenca commented Feb 1, 2024 •
edited
Loading

/cherry-pick 908fd09

@owenca owenca reopened this Feb 1, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 1, 2024
@owenca @llvmbot
[clang-format] Simplify the AfterPlacementOperator option (llvm#79796)
97d44dc 
Change AfterPlacementOperator to a boolean and deprecate SBPO_Never,
which meant never inserting a space except when after new/delete.

Fixes llvm#78892.

(cherry picked from commit 908fd09)
@llvmbot llvmbot mentioned this issue Feb 1, 2024
Merged
@llvmbot
Copy link

llvmbot commented Feb 1, 2024

/pull-request #80259

carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Feb 1, 2024
@owenca
[clang-format] Simplify the AfterPlacementOperator option (llvm#79796)
c320e26 
Change AfterPlacementOperator to a boolean and deprecate SBPO_Never,
which meant never inserting a space except when after new/delete.

Fixes llvm#78892.
@owenca
Copy link
Contributor Author

owenca commented Feb 5, 2024

@tstellar can this be merged before rc2? We need to get the fix into the very first official release of 18.x.

agozillon pushed a commit to agozillon/llvm-project that referenced this issue Feb 5, 2024
@owenca @agozillon
[clang-format] Simplify the AfterPlacementOperator option (llvm#79796)
60eec70 
Change AfterPlacementOperator to a boolean and deprecate SBPO_Never,
which meant never inserting a space except when after new/delete.

Fixes llvm#78892.
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 7, 2024
@owenca @tstellar
[clang-format] Simplify the AfterPlacementOperator option (llvm#79796)
024f45e 
Change AfterPlacementOperator to a boolean and deprecate SBPO_Never,
which meant never inserting a space except when after new/delete.

Fixes llvm#78892.

(cherry picked from commit 908fd09)
@tstellar tstellar closed this as completed Feb 7, 2024
@github-project-automation github-project-automation bot moved this from Needs Fix to Done in LLVM Release Status Feb 7, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
@owenca @tstellar
[clang-format] Simplify the AfterPlacementOperator option (llvm#79796)
14f9da2 
Change AfterPlacementOperator to a boolean and deprecate SBPO_Never,
which meant never inserting a space except when after new/delete.

Fixes llvm#78892.

(cherry picked from commit 908fd09)
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
@owenca @tstellar
[clang-format] Simplify the AfterPlacementOperator option (llvm#79796)
beb8ca3 
Change AfterPlacementOperator to a boolean and deprecate SBPO_Never,
which meant never inserting a space except when after new/delete.

Fixes llvm#78892.

(cherry picked from commit 908fd09)
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
@owenca @tstellar
[clang-format] Simplify the AfterPlacementOperator option (llvm#79796)
24d80cb 
Change AfterPlacementOperator to a boolean and deprecate SBPO_Never,
which meant never inserting a space except when after new/delete.

Fixes llvm#78892.

(cherry picked from commit 908fd09)
tstellar pushed a commit to tstellar/llvm-project that referenced this issue Feb 14, 2024
@owenca @tstellar
[clang-format] Simplify the AfterPlacementOperator option (llvm#79796)
8fc62d1 
Change AfterPlacementOperator to a boolean and deprecate SBPO_Never,
which meant never inserting a space except when after new/delete.

Fixes llvm#78892.

(cherry picked from commit 908fd09)
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
Status: Done
Milestone
LLVM 18.X Release
Development

Successfully merging a pull request may close this issue.

[clang-format] Simplify the AfterPlacementOperator option owenca/llvm-project
5 participants
@tstellar @owenca @EugeneZelenko @HazardyKnusperkeks @llvmbot