Closed Bug 1248470 Opened 5 years ago Closed 5 years ago

NSS clang-format: lib/ssl

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox47 affected)

RESOLVED FIXED
Tracking Status
firefox47 --- affected

People

(Reporter: franziskus, Assigned: franziskus)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Applying clang-format to lib/ssl.
Attached patch LibSslClangFormat.patch (obsolete) — Splinter Review
clang-format on lib/ssl. patch is also at https://codereview.appspot.com/286320043
Flags: needinfo?(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.
Flags: needinfo?(kaie)
What is the difference between this and bug 1204998?
These bugs have almost the same summary.
Attachment #8719578 - Attachment is obsolete: true
(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.
Attached patch LibSslClangFormat.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(kaie)
See Also: → 1249221
Attached patch LibSslClangFormat.patch (obsolete) — Splinter Review
and with the additional protection Kai asked for and I of course forgot to add again.
Attachment #8720690 - Attachment is obsolete: true
(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.
Flags: needinfo?(kaie)
r=kaie on this subset of the patch
Attachment #8720720 - Attachment is obsolete: true
Attachment #8720823 - Flags: review+
Comment on attachment 8720823 [details] [diff] [review]
Subset of Franziskus patch, everything except file ssl3con.c

https://hg.mozilla.org/projects/nss/rev/c2bd9431da86
Attachment #8720823 - Flags: checked-in+
Attached patch ssl3con.diffSplinter Review
clang-format on ssl3con.c
Attachment #8721328 - Flags: review?(kaie)
Attachment #8721328 - Flags: review?(kaie) → review+
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
Attachment #8721328 - Flags: checked-in+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.23
You need to log in before you can comment on or make changes to this bug.