This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Support block indenting array/struct list initializers
ClosedPublic

Authored by gedare on Jun 17 2023, 11:32 AM.

Details

Summary

C89 and C99 list initializers are treated differently than Cpp11 braced initializers. This patch identifies the C array/struct initializer lists by finding the preceding equal sign before a left brace, and applies formatting rules for BracketAlignmentStyle.BlockIndent to those list initializers.

Addresses Issue #57878. https://github.com/llvm/llvm-project/issues/57878

Diff Detail

Event Timeline

gedare created this revision.Jun 17 2023, 11:32 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJun 17 2023, 11:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gedare requested review of this revision.Jun 17 2023, 11:32 AM

It seems to me that there has been a proliferation of new options being proposed and/or accepted recently. I'd like to remind everyone of the long-standing policy of adding new options. That being said, I wonder if we should add a new language LK_C along with its variants C78 (i.e. K&R C), C89 (i.e. ANSI C), GNU extensions (e.g. #55745 and #62755), etc.

It seems to me that there has been a proliferation of new options being proposed and/or accepted recently. I'd like to remind everyone of the long-standing policy of adding new options. That being said, I wonder if we should add a new language LK_C along with its variants C78 (i.e. K&R C), C89 (i.e. ANSI C), GNU extensions (e.g. #55745 and #62755), etc.

Understood. In this case, the BK_ListInit is not visible at the API / Style level. It is an internal categorization to help distinguish different kinds of blocks.

It seems to me that there has been a proliferation of new options being proposed and/or accepted recently. I'd like to remind everyone of the long-standing policy of adding new options. That being said, I wonder if we should add a new language LK_C along with its variants C78 (i.e. K&R C), C89 (i.e. ANSI C), GNU extensions (e.g. #55745 and #62755), etc.

That was already discussed: D117416#3250415

You add a lot of checks and I honestly can't say if it does not affect other code that is not covered by the tests.

clang/lib/Format/FormatToken.cpp
95
101–103

Which clang-format should've added.

MyDeveloperDay added inline comments.Jun 20 2023, 8:33 AM
clang/lib/Format/FormatToken.cpp
95

why pull this out? if you are worried about speed now you do it EVERY time whereas before we wouldn't do it unless limited conditions, for me I'd get rid of bk and just use getBlockKind() directly everytime, the compiler should I hope optimise it away

clang/unittests/Format/FormatTest.cpp
25625

should there be a semi colon here?

25642

should there be a semi colon here?

MyDeveloperDay added inline comments.Jun 20 2023, 8:36 AM
clang/unittests/Format/FormatTest.cpp
25511

nit [2]

MyDeveloperDay edited the summary of this revision. (Show Details)Jun 20 2023, 8:39 AM

In principle I like this as its annoyed me for years

You add a lot of checks and I honestly can't say if it does not affect other code that is not covered by the tests.

Not much I can do about that. I guess it is worth noting that all the checks I add are predicated on either BK_ListInit or BK_BracedInit. I have reduced the checks to what minimally works to address the Issue. Technically, this has two distinct changes included:

  • Allow BlockIndent to work with Braces.
  • Allow BlockIndent to work with array/struct initializers.
gedare updated this revision to Diff 532967.Jun 20 2023, 9:41 AM

Do not use temporary variable for getBlockKind().
Fix test case syntax errors.
Fix formatting.

gedare marked 5 inline comments as done.Jun 20 2023, 9:43 AM
gedare edited the summary of this revision. (Show Details)

My only concern is that this changes behaviour without someone making the conscious decision to make that change, now as much as I can't understand why someone would want to put the } onto the same line, it will only take on person to say "hey I liked it like that" to complain and call it a regression, or that we somehow change the defaults (which people always complain about even through we don't normally do that)

In the past our solution to this problem is to have an option with a "Leave" setting to sort of say, leave it as it currently is. In this case it feels like a bug, looks like a bug and is probably a bug, but I'd be interested in @owenpan and @HazardyKnusperkeks's view

My only concern is that this changes behaviour without someone making the conscious decision to make that change, now as much as I can't understand why someone would want to put the } onto the same line, it will only take on person to say "hey I liked it like that" to complain and call it a regression, or that we somehow change the defaults (which people always complain about even through we don't normally do that)

In the past our solution to this problem is to have an option with a "Leave" setting to sort of say, leave it as it currently is. In this case it feels like a bug, looks like a bug and is probably a bug, but I'd be interested in @owenpan and @HazardyKnusperkeks's view

The github issue basically says all there's to say, if it those constructs should be formatted like function calls, then that's what should happen. Or we should adapt the documentation.

MyDeveloperDay accepted this revision.Jun 21 2023, 10:20 PM
This revision is now accepted and ready to land.Jun 21 2023, 10:20 PM

Can someone land for me? thanks.

My only concern is that this changes behaviour without someone making the conscious decision to make that change, now as much as I can't understand why someone would want to put the } onto the same line, it will only take on person to say "hey I liked it like that" to complain and call it a regression, or that we somehow change the defaults (which people always complain about even through we don't normally do that)

In the past our solution to this problem is to have an option with a "Leave" setting to sort of say, leave it as it currently is. In this case it feels like a bug, looks like a bug and is probably a bug, but I'd be interested in @owenpan and @HazardyKnusperkeks's view

The github issue basically says all there's to say, if it those constructs should be formatted like function calls, then that's what should happen. Or we should adapt the documentation.

There is a warning in the document. (See here.) It should be updated in this patch.

I understand the concern raised by @MyDeveloperDay, but given that BAS_AlwaysBreak already covers the style of breaking after the opening brace but not before the closing brace, I'm okay with this change.

clang/lib/Format/ContinuationIndenter.cpp
365–367

And isBlockIndentedInitRBrace() returns true only if the matching l_brace is of BK_BracedInit or preceded by an =.

1173

All unit tests still pass.

clang/lib/Format/FormatToken.cpp
100–105

This seems redundant and can be deleted.

clang/lib/Format/FormatToken.h
185

We don't need to add a new block kind. Instead, checking if the previous token is = should work.

clang/lib/Format/TokenAnnotator.cpp
859

Either BK_ListInit is superfluous here or a unit test is missing.

4206

Ditto.

5447–5449
clang/unittests/Format/FormatTest.cpp
5042

Ditto below.

gedare updated this revision to Diff 537399.Jul 5 2023, 9:47 AM

Address comments from owenpan.

Do not add the new block type, and instead check explicitly for the equal sign
before an opening right brace.

NOTE: Clang-Format Team Automated Review Comment

Your review contains a change to ClangFormatStyleOptions.rst but not a change to clang/include/clang/Format/Format.h

ClangFormatStyleOptions.rst is auto generated from Format.h via clang/docs/tools/dump_format_style.py, please run this to regenerate the .rst

You can validate that the rst is valid by running.

./docs/tools/dump_format_style.py
mkdir -p html
/usr/bin/sphinx-build -n ./docs ./html
gedare retitled this revision from [clang-format] Add new block type ListInit to [clang-format] Support block indenting array/struct list initializers.Jul 5 2023, 9:49 AM
gedare edited the summary of this revision. (Show Details)
gedare updated this revision to Diff 537402.Jul 5 2023, 9:53 AM

Regenerate docs

gedare requested review of this revision.Jul 5 2023, 9:55 AM
gedare marked 8 inline comments as done.
gedare added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
365–367

yes, this works nicely. thanks for spotting it.

owenpan added inline comments.Jul 5 2023, 2:02 PM
clang/include/clang/Format/Format.h
95–98 ↗(On Diff #537402)
clang/lib/Format/FormatToken.cpp
79–81
85–86
clang/unittests/Format/FormatTest.cpp
25519

Ditto below.

gedare added inline comments.Jul 5 2023, 3:29 PM
clang/include/clang/Format/Format.h
95–98 ↗(On Diff #537402)

This \note is not currently supported? I have submitted a rev D154552 to add such ability.

gedare updated this revision to Diff 537531.Jul 5 2023, 3:43 PM

Address comments

gedare marked 5 inline comments as done.Jul 5 2023, 3:44 PM

Do you need to update the current diff to show the net change? For example, the unit tests you added don't show up as a diff right now.

clang/include/clang/Format/Format.h
95–98 ↗(On Diff #537402)

Ah, you are right. Please still use warning here and update D154552 after this patch lands.

gedare updated this revision to Diff 537761.Jul 6 2023, 9:14 AM

Update and refresh diff

gedare marked an inline comment as done.Jul 6 2023, 9:17 AM

Do you need to update the current diff to show the net change? For example, the unit tests you added don't show up as a diff right now.

Done. Not sure what happened there. But it looks right now.

owenpan accepted this revision.Jul 6 2023, 1:31 PM
This revision is now accepted and ready to land.Jul 6 2023, 1:31 PM
This revision was landed with ongoing or failed builds.Jul 6 2023, 1:42 PM
This revision was automatically updated to reflect the committed changes.