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

Add initial .clang-format style and reformat the sources using it #15

Merged
merged 4 commits into from
Jul 24, 2022

Conversation

oerdnj
Copy link
Contributor

@oerdnj oerdnj commented Jul 22, 2022

This is an initial example of how .clang-format can be configured. It doesn't precisely match the current style, but no tool can do that.

@oerdnj
Copy link
Contributor Author

oerdnj commented Jul 22, 2022

If there's something that does specifically bothers you, I can take a look if that's configurable: https://releases.llvm.org/14.0.0/tools/clang/docs/ClangFormatStyleOptions.html

But I prefer the style to stay consistent across all committers over matching every bit of my preferred style.

There's one nit though - I think that even the single line statements should be surrounded by braces - as a defensive programming, I find these:

        if (obj->parsed_uri->model &&
            strncmp(obj->parsed_uri->model, (const char *)token.model, 16) != 0)
            continue;
        if (obj->parsed_uri->manufacturer &&
            strncmp(obj->parsed_uri->manufacturer,
                    (const char *)token.manufacturerID,
                    32) != 0)
            continue;
        if (obj->parsed_uri->token &&
            strncmp(obj->parsed_uri->token, (const char *)token.label, 32) != 0)
            continue;
        if (obj->parsed_uri->serial && strncmp(obj->parsed_uri->serial,
                                               (const char *)token.serialNumber,
                                               16) != 0)
            continue;

extremely hard to parse.

@simo5
Copy link
Member

simo5 commented Jul 22, 2022

If there's something that does specifically bothers you, I can take a look if that's configurable: https://releases.llvm.org/14.0.0/tools/clang/docs/ClangFormatStyleOptions.html

But I prefer the style to stay consistent across all committers over matching every bit of my preferred style.

Overall I do not mind formatters, but sometimes they make so me silly choices.
Is it possible to make the formatter indent only the changes made to a file but not the entire file in order to preserve exceptions (like so specifically formatted comments) as needed ?

There's one nit though - I think that even the single line statements should be surrounded by braces - as a defensive programming, I find these:

        if (obj->parsed_uri->model &&
            strncmp(obj->parsed_uri->model, (const char *)token.model, 16) != 0)
            continue;
        if (obj->parsed_uri->manufacturer &&
            strncmp(obj->parsed_uri->manufacturer,
                    (const char *)token.manufacturerID,
                    32) != 0)
            continue;
        if (obj->parsed_uri->token &&
            strncmp(obj->parsed_uri->token, (const char *)token.label, 32) != 0)
            continue;
        if (obj->parsed_uri->serial && strncmp(obj->parsed_uri->serial,
                                               (const char *)token.serialNumber,
                                               16) != 0)
            continue;

extremely hard to parse.

I think adding braces here is just fine, I do not mind. In fact I prefer braces even for sinle line if constructs, I just got a bit lazy in this case.

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not too bad, but let's see if it can be improved (I know subjective, but style often is).

src/asymmetric_cipher.c Show resolved Hide resolved
src/asymmetric_cipher.c Show resolved Hide resolved
src/asymmetric_cipher.c Show resolved Hide resolved
src/asymmetric_cipher.c Show resolved Hide resolved
src/asymmetric_cipher.c Outdated Show resolved Hide resolved
src/asymmetric_cipher.c Outdated Show resolved Hide resolved
src/store.c Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
.clang-format Outdated Show resolved Hide resolved
.clang-format Show resolved Hide resolved
@simo5
Copy link
Member

simo5 commented Jul 22, 2022

do you mind if I experiment and rebase your branch ?
I think we can get much closer to some of the nits I don't like.

@oerdnj
Copy link
Contributor Author

oerdnj commented Jul 22, 2022

I'll also just one shot the single line braces. I thought that clang-format does have support for it already but seems it does not, but I can do that with clang-tidy, but it's not that easy to wire it in, so it's more one-time only.

@simo5
Copy link
Member

simo5 commented Jul 22, 2022

I'll probably continue review after the weekend, I think we can get close enough.

@oerdnj
Copy link
Contributor Author

oerdnj commented Jul 22, 2022

do you mind if I experiment and rebase your branch ?

I think we can get much closer to some of the nits I don't like.

I absolutely don't mind, go ahead. I absolutely also mind a little about the exact style, although going full K&R would be a little hard ;).

Once we nail this, there's also a GitHub Action for clang-format or we can write one.

@simo5 simo5 force-pushed the use-clang-format branch 2 times, most recently from f7a10d6 to adbd349 Compare July 23, 2022 14:12
src/exchange.c Outdated Show resolved Hide resolved
@simo5
Copy link
Member

simo5 commented Jul 23, 2022

Not super-happy about how it monkeys with macros, and the whole bizarre things it does with struct initializers with arrays, unless I manually add a comma after the last initializer.

However this comes close enough ...
I am going to try to add a job that checks diffs on pull requests, so that it doesn't complain about intentionally manually arranged stuff.

@simo5 simo5 force-pushed the use-clang-format branch 4 times, most recently from 539bbce to b693ad4 Compare July 23, 2022 15:51
@simo5
Copy link
Member

simo5 commented Jul 23, 2022

Ok, this looks reasonably ok.

The Makefile part is not stellar but gets the job done.

What do you think?

@simo5
Copy link
Member

simo5 commented Jul 23, 2022

Obviously it shows a failure because of the commit where I fixed some clang-format "mistakes".

@oerdnj
Copy link
Contributor Author

oerdnj commented Jul 23, 2022

Obviously it shows a failure because of the commit where I fixed some clang-format "mistakes".

There's a command you can put in the comment to enable/disable formatting of the part of the code.

My experience is that it's simply easier to accept that the tool is not perfect and just let it go...

simo5 and others added 4 commits July 24, 2022 04:58
Without the commas clang-format messes up struct initializers with
odd indentations.
Unfortunately we can't solve this by adding InsertTrailingCommas,
because it cnflicts with bin-packing.
Also fixup some comments as we are going to tell it to leave them alone,
but it will still reformat indentation of the opnening /*

Signed-off-by: Simo Sorce <simo@redhat.com>
Signed-off-by: Ondřej Surý <ondrej@isc.org>
Signed-off-by: Simo Sorce <simo@redhat.com>
Signed-off-by: Simo Sorce <simo@redhat.com>
Fixes latchset#14

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Copy link
Member

simo5 commented Jul 24, 2022

My experience is that it's simply easier to accept that the tool is not perfect and just let it go...

Ok, I guess those macros can live as is ...
When clang will grow some support for better initializer and macro formatting we can revisit this.

@simo5 simo5 merged commit e59cb3e into latchset:main Jul 24, 2022
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.

2 participants