LLNL / axom

CS infrastructure components for HPC applications
BSD 3-Clause "New" or "Revised" License
150 stars 27 forks source link

Explore code style formatting options suitable for Lambdas #146

Open gzagaris opened 4 years ago

gzagaris commented 4 years ago

Explore use of clang-format as the tool of choice for code-formatting. The clang-format tool seems to have some support for lambda and also comes bundled with git-clang-format, which may be used in conjunction with a local pre-commit hook to format files when they are committed.

adrienbernede commented 4 years ago

@gzagaris I already had to deal with moving an existing project to clang-format.

There are some practical questions that need to be answered:

That was the easy case, in any other case, a new formatting policy may have impact on all the project. Two choices then:

Clang-format allows to apply changes on a subset of code. And it is easy to find example of hooks to apply this at commit level, only on the changes introduced by the commit. However, this is not as straightforward as it seems. Clang-format with apply to the changes and their "context" (the preceding and following lines it git diff). Which means the diff will likely change, and the context will be extended, creating a recursive behavior. In short, as an incremental approach, I would recommend applying clang-format per modified files. It would increase the likeliness of a conflict with other contributions, but it’s a trade-off.

I hope that was useful for your "exploration".

gzagaris commented 4 years ago

Thanks @adrienbernede. We're currently using uncrustify but, the tool seems to not recognize Lambdas. It treats them as parameters to functions. In fairness that's what they are, but, the end-result is not what we want the code to look like. There are no uncrustify settings specific to Lambdas.

I think that clang-format has some support for lambdas and that's one of the main reasons we want to look into this. Do you have any experience with that? Moreover, it seems to be easier to setup and we can use it easily with a pre-commit hook.

rhornung67 commented 4 years ago

FWIW, we have been using clang-format in RAJA for several years. It's OK, but formats our test, example, and exercise codes in a way that makes the code less readable, especially with the nested templates we use in our kernel policy API. Thus, we have a script to run clang-format that filters out those directories and we manually format them.

Also, clang-format seems to have very minimal formatting options compared to uncrustify, A-style, and other tools. We will probably switch over to one of those for RAJA in the near future.

gzagaris commented 4 years ago

@rhornung67 , if you find something that works well for RAJA, I think we should follow that for Axom, as well.

rhornung67 commented 4 years ago

Adam has an uncrustify configuration file that he uses in his code that the team likes. We should get that from him and try it out.

gzagaris commented 4 years ago

Sounds good -- I'll poke him about it.

rrsettgast commented 4 years ago

There are no uncrustify settings specific to Lambdas.

Except for

# Add or remove space around '=' in C++11 lambda capture specifications.
#
# Overrides sp_assign.
sp_cpp_lambda_assign            = ignore   # ignore/add/remove/force

# Add or remove space after the capture specification of a C++11 lambda when
# an argument list is present, as in '[] <here> (int x){ ... }'.
sp_cpp_lambda_square_paren      = ignore   # ignore/add/remove/force

# Add or remove space after the capture specification of a C++11 lambda with
# no argument list is present, as in '[] <here> { ... }'.
sp_cpp_lambda_square_brace      = ignore   # ignore/add/remove/force

# Add or remove space after the argument list of a C++11 lambda, as in
# '[](int x) <here> { ... }'.
sp_cpp_lambda_paren_brace       = ignore   # ignore/add/remove/force

# Add or remove space between a lambda body and its call operator of an
# immediately invoked lambda, as in '[]( ... ){ ... } <here> ( ... )'.
sp_cpp_lambda_fparen            = ignore   # ignore/add/remove/force

# If True, cpp lambda body will be indentedDefault=False.
indent_cpp_lambda_body          = false    # false/true

# the value might be used twice:
#   at the assignment
#   at the opening brace
# To prevent the double use of the indentation value, use this option with the value 'True'.
# True:  indentation will be used only once
# False: indentation will be used every time (default).
indent_cpp_lambda_only_once     = true    # false/true

# Don't split one-line C++11 lambdas - '[]() { return 0; }'.
nl_cpp_lambda_leave_one_liners  = true     # false/true

# Add or remove newline between C++11 lambda signature and '{'.
nl_cpp_ldef_brace               = ignore   # ignore/add/remove/force
gzagaris commented 4 years ago

Thanks @rrsettgast , I haven't seen those, is this with a newer version uncrustify? Updating uncrustify may just be sufficient.

rrsettgast commented 4 years ago

@gzagaris Lambda support has been in uncrustify for quite some time. I would grab uncrustify/master since they just fixed an indentation issue with lambda's in nested namespaces. Those settings I listed give OK results so give them a shot.

gzagaris commented 4 years ago

Will do, thanks!