Page MenuHomePhabricator

[clang-format] Add SpaceBeforeBrackets
ClosedPublic

Authored by Anteru on Jan 11 2015, 6:54 AM.

Details

Summary

Adds a new option SpaceBeforeBrackets to add spaces before brackets (i.e. int a[23]; -> int a [23];) This is present as an option in the Visual Studio C++ code formatting settings, but there was no matching setting in clang-format.

Diff Detail

Event Timeline

Anteru updated this revision to Diff 17989.Jan 11 2015, 6:54 AM
Anteru retitled this revision from to [clang-format] Add SpaceBeforeBrackets.
Anteru updated this object.
Anteru edited the test plan for this revision. (Show Details)
Anteru added a subscriber: Unknown Object (MLST).
djasper edited edge metadata.Apr 16 2015, 1:38 AM

Sorry it took me so long to reply.

include/clang/Format/Format.h
356–357 ↗(On Diff #17989)

To be consistent with the option above, this should probably be called SpaceBeforeSquareBrackets. This is to make clear that this is not considering angle brackets or curly brackets.

However, I am actually not sure that is the best name. I think you are only trying to change the behavior for array subscripts. Maybe we should name the option accordingly (SpaceBeforeArraySubscripts). We can also rename SpaceInSquareBrackets correspondingly (I'll do that in a separate follow-up patch as it is a little bit tedious to be backwards compatible).

unittests/Format/FormatTest.cpp
7992 ↗(On Diff #17989)

If this is only targeting array subscripts as I presume, please add corresponding tests that it doesn't influence other behavior, specifically lambdas and objc method expressions (e.g. "f([] {});").

Also not that array subscripts can be written the other way around "1[a]" is identical to "a[1]". Add a test for that as well.

Anteru updated this revision to Diff 35810.EditedSep 26 2015, 12:59 PM
Anteru edited edge metadata.

Here's an updated diff which should do the trick. If not, I'll fix again :)

MyDeveloperDay set the repository for this revision to rG LLVM Github Monorepo.
MyDeveloperDay added projects: Restricted Project, Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2019, 12:16 PM

I realize this is an old diff, and you and I have just spoke about it on twitter https://twitter.com/NIV_Anteru/status/1193792307386081281?s=20 would you consider rebasing as the /brief style isn't used any more

It would help the approval process if you could highlight a public project with a style guide that uses this style.

I think this is a simple enough addition to consider by getting it upto date will help

include/clang/Format/Format.h
415 ↗(On Diff #35810)

when you rebase you'll see examples of /codeblocks with before and after I think that would really help

Anteru updated this revision to Diff 229612.Nov 15 2019, 11:56 AM
Anteru edited the summary of this revision. (Show Details)

Updated as requested.

MyDeveloperDay accepted this revision.Nov 15 2019, 12:18 PM

Thank you for this patch. I'm sorry it took so long, I think this is a perfectly reasonable idea and helps cover the functionality that visual studio also supports. (which ultimately will help visual studio users embrace clang-format even more than they are already)

This LGTM, and thank you, let us know if you need someone to land this for you.

This revision is now accepted and ready to land.Nov 15 2019, 12:18 PM
Anteru added a comment.EditedNov 15 2019, 1:40 PM

Thanks a lot! And yes, I need someone to help me land this :) -- I have no idea how to land it myself.

This revision was automatically updated to reflect the committed changes.

fyi, +x permissions was added to clang/lib/Format/Format.cpp
I reverted the change in a4a7c1259e8a8f2d11fa29686a6c2834948c1358

Would be nice to add this to the release notes too ;)

Would be nice to add this to the release notes too ;)

Addressing this and other missing release notes in D70355: [clang-format] [NFC] add recent changes to release notes, want to try and bring about a change were new clang-format changes are always release noted as I've seen done with clang-tidy

@Anteru thanks for the patch this got landed today, I got burnt by the 100755 mode change on the file, but thanks to @sylvestre.ledru it got resolved.