Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce horizontal space by formatting short/empty functions to single line #86

Open
Flamefire opened this issue Jun 4, 2020 · 4 comments

Comments

@Flamefire
Copy link
Contributor

Currently there are 3 cases I'd like to change:

int foo()
{
  return x;
}
void bar()
{
}
struct Baz
{
}

Those come up in setters/getters and the last one for type traits and similar.

@AndreasGocht Any objection to making some or all of them be put on a single line? I find that much easier to read and less annoying to scroll. The clang-format setting is BreakBeforeBraces set to Custom and defining the above case to not break.

@AndreasGocht
Copy link
Collaborator

Actually yes. The .clang-fromat is copied from the ZIH Wiki [internal wiki], which is an approach to have a unique style about different projects. I think @tilsche introduced this. I would like to have the .clang-fromat file consistent with the wiki.

Best,
Andreas

@Flamefire
Copy link
Contributor Author

Ok sure, that is a good reason. Then I'd like to propose a change to that internal style that allows empty class braces on 1 line. For uncrustify this is nl_class_leave_one_liners and for clang-format it is SplitEmptyRecord in BraceWrapping

The use case comes up mainly in template meta-programming so I believe that case simply didn't occur when designing the format and most existing code won't change. However when using TMP it improves readability IMO.

Maybe @tilsche could comment on that too?

@tilsche
Copy link

tilsche commented Jun 5, 2020

In general, I don't mind refining the style. However, we use this style in many of repositories across different sites. It's painful to keep consistent and introduces style changes on quite a body of code. So a change should contribute a significant improvement to make it worth it.

Of course if a repo has substantially different characteristics, it could also be argued for using an alternate style.

I don't see the point in the context of this repo. I don't see any code where it would even make a difference. Maybe a specific code example would help make the point?

As a matter of personal taste, I lean towards verbosity and I don't primarily navigate by vertical scrolling.

@Flamefire
Copy link
Contributor Author

I don't expect this change to change much existing code because, as you noticed in this repo, empty classes are rarely used. They usually come up as traits for TMP, especially in C++11 (due to std::true_type/false_type being available there)

An example is this: https://github.com/Flamefire/scorep_binding_python/blob/58ca970b3316e0ed1bd242dcfbb1a3aae4be9642/src/scorepy/pythonHelpers.hpp#L92-L99

This is from a feature I'm working on where some TMP is used. I could imagine similar cases to pop up in otf2cpp. I don't see the benefit of having the closing brace at its own line

As the impact is very limited a point can be made for both changing the style and keeping it the way it is. My suggestion would be to add it to the wiki but don't update existing projects (besides this) just for that. This way new projects will use it too and in case existing ones update the style file they get it later leading to a couple newlines removed if it changes anything it all

Sure coding style is highly influenced by personal taste and in the end it doesn't matter which exact style is used. In this case I find the collapsed braces more readable especially as it puts related specializations closer together so you can see all of them more easily.

Anyway feel free to close this issue if you disagree. I opened this because I feel this is rather an oversight than a conscious decision as traits didn't occur yet and wanted to bring it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants