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

Fine tune clang-format proposal #2290

Merged
merged 4 commits into from
Feb 20, 2023
Merged

Conversation

AntoinePrv
Copy link
Member

@AntoinePrv AntoinePrv commented Feb 14, 2023

This adds new options to clang-format as well as change a few defaults.

    1. Break lines/functions in Python black style, with a semantic level of indentation rather than aligning to the arbitrary column of the open (;
    1. Add empty line after access modifiers (private, public...)
    1. Apply curly braces around control statements
    1. Reorder qualifiers (const, inline, ...) (with const to the left :'( but it seems more accepted)
    1. Sort and group includes

I know it makes a big diff that will create conflicts in PRs, but I think it improve readability.
Let me know what you think (not all formatting end up looking nice, some may need manual rewrite).

@Klaim
Copy link
Member

Klaim commented Feb 14, 2023

Personally I'm ok with most of the style changes/tweaks here.

The one thing I'm not a fan of, because I think it doesnt help readability and sometime makes it worse is adding scopes to one-liner if or loop statements. In particular when the statement is continue;, break; or return;. In my experience it's best left to be decided by the writer(s).
BUT I am not against trying it either so if there is a push to keep that one I'm ok with it.

@Hind-M
Copy link
Member

Hind-M commented Feb 14, 2023

I don't know if it's out of habit, but indenting function arguments on the same level of the function name, and the carriage return after the first argument can be confusing and maybe less readable?
Otherwise, the other changes are nice, especially the priority on keywords ^^

@AntoinePrv
Copy link
Member Author

is adding scopes to one-liner if or loop statements

@Klaim I've seen it being recommended a few times, as it is helps to avoids bugs when so comes and adds a second line. True it does not change much around continue... but then it's not automated.

indenting function arguments on the same level of the function name, and the carriage return after the first argument can be confusing and maybe less readable

@Hind-M do you prefer the old or new style (or another yet)?

@Hind-M
Copy link
Member

Hind-M commented Feb 15, 2023

@Hind-M do you prefer the old or new style (or another yet)?

I prefer this one:

void func(arg1,
          arg2,
          arg3,
          ...);

instead of something like:

void func(
     arg1,
     arg2,
     arg3,
     ...);

@AntoinePrv
Copy link
Member Author

@Hind-M, we currently have this

very_long_type very_long_function_name_func(arg1,
                                            other_long_func(very_long_arg_two),
                                            arg3,
                                            ...);

The proposal is to switch to

very_long_type very_long_function_name_func(
    arg1,
    other_long_func(very_long_arg_two),
    arg3,
    ...
);

But indeed not this

very_long_type very_long_function_name_func(
               arg1,
               other_long_func(very_long_arg_two),
               arg3,
               ...);

Does that look more reasonable?

@Hind-M
Copy link
Member

Hind-M commented Feb 15, 2023

Does that look more reasonable?

Hmm I still prefer what we have now, but as I said maybe it's because I'm used to it :)
Btw and out of curiosity, is there a reason for this specific change? (consistency with rules elsewhere, recommended,...)
Can also be a matter of taste of course.

@AntoinePrv
Copy link
Member Author

AntoinePrv commented Feb 15, 2023

is there a reason for this specific change?

  • Item i is a strong preference of mine, as I find it hard to read when the names are long and get aligned far to the right. It is also consistent with Python Black.
  • Item ii are preferences of @SylvainCorlay and @JohanMabille
  • Item iii is recommendation against bug-prone patterns.
  • Item iv for consistency; order preference of @JohanMabille
  • Item v is a suggestion (I'm open to other)

@Klaim
Copy link
Member

Klaim commented Feb 15, 2023

Personally I prefer the proposed indentation, because the current one means there is a lot of code on the right that's a lot of space taken for nothing and I am also more used to something closer to the proposed indentation. In my own projects though I use something closer to the third version rejected, but less expansive on space, but that's anecdotal.

@Klaim I've seen it being recommended a few times, as it is helps to avoids bugs when so comes and adds a second line. True it does not change much around continue... but then it's not automated.

Yeah the automatisation helps use write like this, but I'm not convinced it will prevent any bug due to misreading the code. However as said I'm willing to try so ok.

@wolfv
Copy link
Member

wolfv commented Feb 20, 2023

LGTM!

@AntoinePrv
Copy link
Member Author

@jonashaag do you agree with these changes?

@AntoinePrv AntoinePrv force-pushed the clang-format branch 2 times, most recently from 2210500 to 986b36f Compare February 20, 2023 11:32
@AntoinePrv AntoinePrv merged commit 365342c into mamba-org:main Feb 20, 2023
AntoinePrv added a commit that referenced this pull request Feb 20, 2023
@AntoinePrv AntoinePrv deleted the clang-format branch February 27, 2023 14:39
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

Successfully merging this pull request may close these issues.

4 participants