Page MenuHomePhabricator

[clang-format] Add style option AllowShortLambdasOnASingleLine
ClosedPublic

Authored by rdwampler on Feb 4 2019, 7:24 AM.

Details

Summary

This option AllowShortLambdasOnASingleLine similar to the other AllowShort* options, but applied to C++ lambdas.

I considered making this dependent on AllowShortFunctionsOnASingleLine, but could not think of how the breaking should be behave when the option is set to Inline and InlineOnly.

I could not find a specific coding style that requires short lambdas to always break on a new line, but I do think it's fairly common practice. Also, I note that the patch: https://reviews.llvm.org/D44609 attempts to add an option to allow formatting lambda using the Allman style braces. Here is a coding style that requires Allman style braces, but allows lambdas to be on single line if they are short. So, maybe this option would be required to handle this situation?

Addresses: https://bugs.llvm.org//show_bug.cgi?id=32151

Diff Detail

Repository
rL LLVM

Event Timeline

rdwampler created this revision.Feb 4 2019, 7:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 7:24 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rdwampler updated this revision to Diff 185848.Feb 7 2019, 1:01 PM

Update to include an option Inline to only put short lambda on a single line if used as an argument. See the following code style guide from catboost: ps://github.com/catboost/catboost/blob/master/CPP_STYLE_GUIDE.md#lambda-functions

I'm not sure if Inline is the best name. I welcome suggestions.

Thanks!

Could you add a line in the docs/ReleaseNotes.rst to say what you are adding

MyDeveloperDay added inline comments.Feb 21 2019, 2:11 AM
clang/lib/Format/Format.cpp
649 ↗(On Diff #187260)

What is the difference between what clang-format does now and using SLS_All, i.e. if your introducing a new style shouldn't the default be to not change the exsiting code?

Without trying this myself I would think this needs to be SLS_None? (am I wrong?)

rdwampler marked an inline comment as done.Mar 4 2019, 8:39 AM
rdwampler added inline comments.
clang/lib/Format/Format.cpp
649 ↗(On Diff #187260)

AFAICT, the current behavior is to always put lambdas on a single line, if possible.

It might be a good idea to add the nested lambda tests from D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style) into your review to show how this patch copes with those (and the examples from the comments of that review)

klimek added inline comments.Mar 20 2019, 6:05 AM
clang/lib/Format/TokenAnnotator.cpp
2976 ↗(On Diff #187260)

If SLS_All is supposed to not change anything, should we fall through here if (SLS_All)?

rdwampler updated this revision to Diff 191629.Mar 20 2019, 8:57 PM
rdwampler marked 2 inline comments as done.

Changes since last revision:

-rebased
-Fall through when SLS_All.

rdwampler added inline comments.Mar 20 2019, 8:58 PM
clang/lib/Format/TokenAnnotator.cpp
2976 ↗(On Diff #187260)

Yes, I think that's a better approach to ensure the current behavior is unchanged.

rdwampler updated this revision to Diff 191872.Mar 22 2019, 6:36 AM

Rebased on master.

This revision is now accepted and ready to land.Mar 22 2019, 7:12 AM

I don't have commit rights, can someone land this for me or would it be better to try and get commit access?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 1:16 PM