1248470 - NSS clang-format: lib/ssl

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

Applying clang-format to lib/ssl.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: LibSslClangFormat.patch

clang-format on lib/ssl. patch is also at https://codereview.appspot.com/286320043

Comment 2

[Kai Engert (:KaiE:)]

lib/ssl/sslcon.c

please add /* clang-format off */ to avoid reformat of these arrays:
- static const PRUint8 implementedCipherSuites[ssl2_NUM_SUITES_IMPLEMENTED * 3] = {
- static const ssl2Specs ssl_Specs[] = {


lib/ssl/sslimpl.h

Please consider to manually fix the following array, and add /* clang-format off */,
as it seems nice if the bit sizes are aligned vertically, too:
- typedef struct sslOptionsStr {


Everything else in the patch looks acceptable to me.

If it's simpler, I'm willing to r+ this patch, if you promise to immediately
attach a follow-up patch that performs these protections and reverts the
formatting to the previous nice layout.

Comment 3

[Takanori MATSUURA]

What is the difference between this and bug 1204998?
These bugs have almost the same summary.

Comment 4

[Kai Engert (:KaiE:)]

(In reply to Takanori MATSUURA from comment #3)
> What is the difference between this and bug 1204998?
> These bugs have almost the same summary.

I thin Franziskus wasn't aware of that bug.

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: LibSslClangFormat.patch

I've seen the other bug, then forgot about it because it was so messy already, and then created this one... 

Anyway, this patch produces the same binaries as without the patch but does not touch ssl3con.c other than removing trailing whitespaces. I'll file a follow up to investigate what's going on there. But if you can r+ this one Kai we can get it checked in.

Comment 6

[Franziskus Kiefer [:franziskus]]

Attachment: LibSslClangFormat.patch

and with the additional protection Kai asked for and I of course forgot to add again.

Comment 7

[Kai Engert (:KaiE:)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #6)
> Created attachment 8720720 [details] [diff] [review]
> LibSslClangFormat.patch


I think something went wrong with your attempt to format ssl3con.c separately.
If you open this file in an editor and look at line 6637, you'll see an example for an if body that doesn't get indented. I see several such places in ssl3con.c

I suggest that you take more time with ssl3con.c and postpone that to a future patch.

Thanks for identifying that only reformatting of ssl3con.c causes the binary to be different.

I'm giving you a partial r=kaie for your patch - for everything except your changes to ssl3con.c

I'll check in that subset. Let's handle the ssl3con.c in a separate follow-up patch.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Subset of Franziskus patch, everything except file ssl3con.c

r=kaie on this subset of the patch

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 8720823 [details] [diff] [review]
Subset of Franziskus patch, everything except file ssl3con.c

https://hg.mozilla.org/projects/nss/rev/c2bd9431da86

Comment 10

[Franziskus Kiefer [:franziskus]]

Attachment: ssl3con.diff

clang-format on ssl3con.c

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 8721328 [details] [diff] [review]
ssl3con.diff

Checked in, despite the potential issue discussed in bug 1249221.
Let's test if we discover any regressions.

https://hg.mozilla.org/projects/nss/rev/a3ed8b6cfc79