[Lint] One parameter/argument per line for C++ code

[kfstorm]

Why are these changes needed?

It's really annoying to deal with parameter/argument conflicts. This is even frustrating when we merge code from the community to Ant's internal code base with hundreds of conflicts caused by parameters/arguments.

In this PR, I updated the clang-format style to make parameters/arguments stay on different lines if they can't fit into a single line.

There are several benefits:

• Conflict resolving is easier.
• Less potential human mistakes when resolving conflicts.
• Git history and Git blame are more straightforward.
• Better readability.
• Align with the new Python format style.

Related issue number

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[raulchen]

I'm strongly in favor of putting one argument per line. The benefit is that this makes the git history very clear. E.g., you can easily find which PR introduced a given parameter.
I used to comment this issue repeatedly. Glad that clang can automatically do this.

@ericl @scv119 any comments?

[ericl]

Hmm, personally I think the ugliness is a bit much if the problem could be solved in other ways. Maybe this is a sign we need narrower / cleaner interfaces in Ray for bits that need to be extended, or more components should be upstreamed?

[scv119]

cc @iycheng @mwtian
personally I like the old formatting as it makes code more compact, but I not have a strong opinion here. Also agree with @ericl here it might mean the interfaces need to be narrowed.

[fishbone]

I think the original one seems better. Not sure, but for things like this:

  return ray::internal::Serializer::Deserialize<std::shared_ptr<T>>(
      packed_object->data(), packed_object->size());

to be formatted like this:

  return ray::internal::Serializer::Deserialize<std::shared_ptr<T>>(
      packed_object->data(),
      packed_object->size());

seems really wired.

I checked gRPC and tensorflow and they are not using this one and I'm not sure whether they have similar issues like the one described or not. It seems like other things are not working well, like what Eric mentioned.

I'll vote readability over the things mentioned in the descriptions.

[kfstorm]

I tried these style options a few months ago, and I didn't go through each change one by one. I agree this PR includes some unnecessary changes. We just want to raise the issue of long parameter lists.

Ideally, we can upstream more code (we've planned) and refine interfaces, but it's not a thing that can be done within months.

I like the idea to put parameters one per line if they can't fit into a single line. Consider changing this

  local_raylet_client_ = std::make_shared<raylet::RayletClient>(
      io_service_, std::move(grpc_client), options_.raylet_socket, GetWorkerID(),
      options_.worker_type, worker_context_.GetCurrentJobID(), options_.runtime_env_hash,
      options_.language, options_.node_ip_address, &raylet_client_status,
      &local_raylet_id, &assigned_port, &serialized_job_config, options_.startup_token);

to

local_raylet_client_ = std::make_shared<raylet::RayletClient>(
      io_service_,
      std::move(grpc_client),
      options_.raylet_socket,
      GetWorkerID(),
      options_.worker_type,
      worker_context_.GetCurrentJobID(),
      options_.runtime_env_hash,
      options_.language,
      options_.node_ip_address,
      &raylet_client_status,
      &local_raylet_id,
      &assigned_port,
      &serialized_job_config,
      options_.startup_token);

The latter one is way more readable and maintainable to me.

I really want to know your opinion about formatting style. Maybe you can reply with a number:

1. The current formatting is fine. Nothing needs to be changed.
2. I like the idea to put parameters one per line if they can't fit into a single line, but this PR has some unnecessary changes / I don't like the new style.
3. This PR is great, and I have even more ideas about formatting.
4. None of the above. Please state your opinion.

[raulchen]

Regarding readability and aesthetics, I agree that some changes in this PR are not ideal. IIRC, Python's rule is that if all parameters can fit into a single line, put them in a single line, otherwise put each in one line. If we can adopt the same rule, I think this PR can improve code readability. And the amount of changes should be much less. @iycheng will this address your concern?

Regarding code conflicts, we are indeed reviewing all existing diffs and planing to upstream as many existing diffs as possible throughout this year. If we could improve this format issues, it would greatly facilitate the process.

Last but not least, I'd like to emphasize the benefit of clean git history again. I'v seen such cases many times. It was very inconvenient to git blame the real commit.

[fishbone]

I'm not 100% sure about the rules there. But I think if in the end, for functions which have more than 5 parameters, we can put them line-by-line maybe it's ok. I also think for this case, we probably should do it like:

fun_call(
  param1, /* name1 */
  param2, /* name2 */
  param3, /* name3 */
  param4, /* name4 */
  param5, /* name5 */
  param6 /* name6 */
)

Also, I don't think cpp's style has to be aligned with python's style. They are different languages.

[jjyao]

Just my opinion: the python rule sounds good to me (and the clean git blame).

Could we apply python rule and update the PR so people can see how it looks in real?

Something I came across: https://softwareengineering.stackexchange.com/questions/88485/is-it-a-bad-idea-to-list-every-function-method-argument-on-a-new-line-and-why

[scv119]

Regarding readability and aesthetics, I agree that some changes in this PR are not ideal. IIRC, Python's rule is that if all parameters can fit into a single line, put them in a single line, otherwise put each in one line. If we can adopt the same rule, I think this PR can improve code readability.

I think this would be ideal, I'm sold on this.

[mwtian]

I'm ok as long as it is consistent across the code base for most C++ constructs, and similar to a fraction of other C++ OSS projects.

One disadvantage of the new style is that when reading code, more vertical screen estate will be used for the same amount of information.

[mwtian]

This option seems only useful for Javascript? https://clang.llvm.org/docs/ClangFormatStyleOptions.html

[kfstorm]

Thanks. I'll revisit all configurations.

[mwtian]

Is this consistent, with AllowAllConstructorInitializersOnNextLine to true and other *OnNextLine settings false? folly seems to set BinPack* to false and *OnNextLine to true.

[kfstorm]

Everyone, I've revisited the configurations and simplifed them to avoid unnecessary changes.

Simple stats for comparison:

The original style and stats:

BinPackArguments: false
BinPackParameters: false
AllowAllArgumentsOnNextLine: false
AllowAllParametersOfDeclarationOnNextLine: false
InsertTrailingCommas: Wrapped
AlignAfterOpenBracket: AlwaysBreak
AllowAllConstructorInitializersOnNextLine: true
ConstructorInitializerAllOnOneLineOrOnePerLine: true

449 files changed, 22015 insertions(+), 12225 deletions(-)

The new style and stats:

BinPackArguments: false
BinPackParameters: false

377 files changed, 9807 insertions(+), 5166 deletions(-)

[kfstorm]

I'm not 100% sure about the rules there. But I think if in the end, for functions which have more than 5 parameters, we can put them line-by-line maybe it's ok. I also think for this case, we probably should do it like:

fun_call(
  param1, /* name1 */
  param2, /* name2 */
  param3, /* name3 */
  param4, /* name4 */
  param5, /* name5 */
  param6 /* name6 */
)

Also, I don't think cpp's style has to be aligned with python's style. They are different languages.

I can't find a configuration to have different formatting behaviors based on the count of parameters.

[mwtian]

Nice that we can trim down the formatting rule change and code diff. There are other cost to the change, e.g. deviation from the base Google style, disruption to existing PRs etc. But LGTM if there is consensus on this.