[PATCH] D129443: [clang-format] Add option for aligning requires clause body

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9c422ab7ce82: [clang-format] Add option for aligning requires clau

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

HazardyKnusperkeks added a comment. In D129443#3867400 , @owenpan wrote: > In D129443#3867364 , @rymiel wrote: > >> How should I proceed with a stale rejecting review? > > You can commit as https://reviews.llvm.or

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

owenpan added a comment. In D129443#3867364 , @rymiel wrote: > How should I proceed with a stale rejecting review? You can commit as https://reviews.llvm.org/D129443#3641571 has been addressed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

rymiel added a comment. How should I proceed with a stale rejecting review? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129443/new/ https://reviews.llvm.org/D129443 ___ cfe-commits mailing list cfe-com

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

rymiel updated this revision to Diff 468414. rymiel added a comment. Swap the default for LLVM style, and adjust tests which depended on the previous style to still use it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129443/new/ https://reviews.

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

HazardyKnusperkeks added a comment. In D129443#3857816 , @owenpan wrote: > In D129443#3857608 , @rymiel wrote: > >> Changing the default LLVM style would change the output of all the >> requires-related test case

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

owenpan added a comment. In D129443#3857608 , @rymiel wrote: > Changing the default LLVM style would change the output of all the > requires-related test cases in `FormatTest.Concepts`. Should I change these > test cases to use the new indentation or pa

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

rymiel added a comment. Changing the default LLVM style would change the output of all the requires-related test cases in `FormatTest.Concepts`. Should I change these test cases to use the new indentation or pass the `REI_Keyword` style to them? Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

owenpan added inline comments. Comment at: clang/lib/Format/Format.cpp:1304 LLVMStyle.RequiresClausePosition = FormatStyle::RCPS_OwnLine; + LLVMStyle.RequiresExpressionIndentation = FormatStyle::REI_Keyword; LLVMStyle.SeparateDefinitionBlocks = FormatStyle::SDS_Leave;

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

HazardyKnusperkeks added inline comments. Comment at: clang/lib/Format/Format.cpp:1304 LLVMStyle.RequiresClausePosition = FormatStyle::RCPS_OwnLine; + LLVMStyle.RequiresExpressionIndentation = FormatStyle::REI_Keyword; LLVMStyle.SeparateDefinitionBlocks = FormatStyle::SDS_

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

owenpan added inline comments. Comment at: clang/docs/ReleaseNotes.rst:540 -- Add `RemoveSemicolon` option for removing `;` after a non-empty function definition. +- Add ``RemoveSemicolon`` option for removing ``;`` after a non-empty function definition. +- Add ``

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

HazardyKnusperkeks added inline comments. Comment at: clang/docs/ReleaseNotes.rst:540 -- Add `RemoveSemicolon` option for removing `;` after a non-empty function definition. +- Add ``RemoveSemicolon`` option for removing ``;`` after a non-empty function definition

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

owenpan added a comment. In D129443#3847319 , @rymiel wrote: > Note that I didn't change the LLVM config, which right now is still set to > `Keyword`. We should change it back to `OuterScope`. Comment at: clang/lib/Format/Format.cpp:

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

rymiel marked 2 inline comments as done. rymiel added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:20036 Style.Language = FormatStyle::LK_Cpp; + CHECK_PARSE_BOOL(AlignRequiresClauseBody); CHECK_PARSE_BOOL(AlignTrailingComments); owenp

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

rymiel updated this revision to Diff 466551. rymiel added a comment. Flip the default and note that in the Release Notes Sorry, I did not realize I was mentioned. I think the default option will always be the first option? So I swapped them around. Note that I didn't change the LLVM config, whic

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

Vigilans accepted this revision. Vigilans added a comment. In D129443#3844805 , @HazardyKnusperkeks wrote: > In D129443#3844535 , @owenpan wrote: > >> In D129443#3842427

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

HazardyKnusperkeks added a reviewer: Vigilans. HazardyKnusperkeks added a comment. In D129443#3844535 , @owenpan wrote: > In D129443#3842427 , > @HazardyKnusperkeks wrote: > >> We can //fix// that regression, and

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

owenpan added a comment. In D129443#3842427 , @HazardyKnusperkeks wrote: > We can //fix// that regression, and I will adopt this change for my company > to keep the requires expressions where they are. :) Then we have three options: 1. Only fix the "r

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

HazardyKnusperkeks added a comment. In D129443#3841248 , @owenpan wrote: > Is this option really required? Can we just fix the regression as reported in > https://github.com/llvm/llvm-project/issues/56283? It seems that we haven't > followed the policy

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

rymiel added a comment. It wasn't an accidental regression, though, it was a conscious formatting decision (maybe ask @HazardyKnusperkeks for more). Similar to how lambdas can be formatted aligned to the introducer, i think this option does make sense. Repository: rG LLVM Github Monorepo CH

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

owenpan added a comment. Is this option really required? Can we just fix the regression as reported in https://github.com/llvm/llvm-project/issues/56283? It seems that we haven't followed the policy late

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

rymiel updated this revision to Diff 463773. rymiel added a comment. Missed a spot when re-sorting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129443/new/ https://reviews.llvm.org/D129443 Files: clang/docs/ClangFormatStyleOptions.rst clang/i

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

rymiel updated this revision to Diff 463772. rymiel added a comment. Resolve nits: - Re-sorted uses of the name RequiresExpressionIndentationKind - Updated version to be 16 - Add test for demonstrating indentation as subexpression - Reformatted Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

rymiel commandeered this revision. rymiel added a reviewer: eoanermine. rymiel added a comment. Commandeering to finish the final nits as per https://github.com/llvm/llvm-project/issues/56283#issuecomment-1261653291 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revi

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

HazardyKnusperkeks added a comment. Looks very promising. Comment at: clang/include/clang/Format/Format.h:396 + /// \version 15 + RequiresExpressionIndentationKind RequiresExpressionIndentation; + Please resort. Comment at: clang/include/cl

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

eoanermine updated this revision to Diff 444556. eoanermine edited the summary of this revision. eoanermine added a comment. - Rename AlignRequiresClauseBody to RequiresExpressionIndentation - Introduce RequiresExpressionIndentationKind as a type of RequiresExpressionIndentation - Add more tests

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

HazardyKnusperkeks added a comment. In D129443#3645795 , @Vigilans wrote: > In my knowledge of clang-format, `Requires Clause` and `Requires Expression` > are treated differently. > +1 for the enum, even if we only have 2 options yet. And yes, the op

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

Vigilans added a comment. In my knowledge of clang-format, `Requires Clause` and `Requires Expression` are treated differently. In cppreference , A `Requires Clause` is specific to the `requires` keyword used for constraints on template a

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

eoanermine added a comment. In D129443#3644795 , @eoanermine wrote: > Add tests for AlignRequiresClauseBody option Are these tests fine? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129443/new/ https://

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

eoanermine updated this revision to Diff 443895. eoanermine added a comment. Add tests for AlignRequiresClauseBody option Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129443/new/ https://reviews.llvm.org/D129443 Files: clang/include/clang/Forma

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

eoanermine added a comment. In D129443#3641571 , @curdeius wrote: > Haven't you forgotten to add formatting tests? :) Yes, I have. Thank you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129443/new/ http

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

HazardyKnusperkeks added a comment. Thanks for the patch. Please add the full context for the next revision: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129443/new/

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

curdeius requested changes to this revision. curdeius added a comment. This revision now requires changes to proceed. Haven't you forgotten to add formatting tests? :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129443/new/ https://reviews.llvm.o

[PATCH] D129443: [clang-format] Add option for aligning requires clause body

eoanermine created this revision. eoanermine added projects: clang, clang-format. Herald added a project: All. eoanermine requested review of this revision. Herald added a subscriber: cfe-commits. - Add an option whether requires clause body should be aligned with `requires` keyword Fixes https: