Page MenuHomePhabricator

clang-format: Use block indentation for braced initializations

Authored by DaanDeMeyer on Mar 15 2020, 8:29 AM.



Currently, when UseTab is set to ForIndentation, the following code is indented using a tab for the struct indentation and spaces for the braced init indentation:

static const struct {
	const char *name; # This line starts with a tab
	int value;
} keywords[] = {
	{"build", BUILD}, # This line starts with 4 spaces
	{"default", DEFAULT},
	{"include", INCLUDE},
	{"pool", POOL},
	{"rule", RULE},
	{"subninja", SUBNINJA},

I believe the braced init lines should start with a tab as well in this case.

When trying to modify clang-format to make this happen, I discovered braced init lists are currently marked with the BK_Unknown block type so I modified the code to assign the BK_BracedInit block type to l_brace tokens that start a braced init. I also modified the method that decides whether to use block indents to take braced inits into account.

I'm completely unfamiliar with the clang-format codebase so these might not be the best changes to achieve the desired affect. I'm also not entirely sure everyone will agree that a tab should be used here so I'm submitting this first for feedback before adding/fixing the tests.

Diff Detail

Event Timeline

DaanDeMeyer created this revision.Mar 15 2020, 8:29 AM

Changed the implementation to simply remove the handling of l_brace in tok::equal. The state machine simply continues executing and the next case will be the l_brace case which first tries to parse the l_brace as a braced init list.

Eugene.Zelenko edited reviewers, added: MyDeveloperDay; removed: Restricted Project.Mar 15 2020, 9:19 AM
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
Eugene.Zelenko added a subscriber: cfe-commits.
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2020, 9:19 AM

New implementation that breaks fewer tests.

It seems to be expected that block kind is BK_Unknown for C struct braced init lists. Is that supposed to be the case?

There's still quite some tests failures but most of them are simply indentation changes from 4 to 2 spaces (which is LLVM's indentation size) which might be unavoidable if we want this change to happen.

MyDeveloperDay requested changes to this revision.Mar 26 2020, 7:34 AM

New implementation that breaks fewer tests.

fewer is not an option, no is all thats going to get accepted.

Please add tests for you own use case

This revision now requires changes to proceed.Mar 26 2020, 7:34 AM

Of course, if there's interest in adding this I'll fix all the tests but I wanted to make sure there was interest in adding this since it changes clang-format's behavior. Can you confirm that this change in behavior is a good thing and might be merged if I fix all the tests? If the existing output cannot be changed in any way then this revision can be closed since it fundamentally requires changing clang-format's output.

DaanDeMeyer abandoned this revision.Apr 26 2020, 2:36 AM

Turns out this is already possible by disabling Cpp11BracedListStyle. The proposed change introduced too many changes regardless so I'm abandoning this.