bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

NSS clang-format: lib/base

RESOLVED FIXED in 3.22

Status

NSS
Libraries
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fkiefer, Assigned: fkiefer)

Tracking

(Blocks: 1 bug)

trunk
3.22

Firefox Tracking Flags

(firefox44 affected)

Details

Attachments

(3 attachments)

Applying clang-format to lib/base.
Created attachment 8678907 [details] [diff] [review]
protection

disable clang-format on parts of the code
Created attachment 8678916 [details] [diff] [review]
lib/base clang-format

clang-format for lib/base. also on codereview [1].

[1] https://codereview.appspot.com/273050043/
Attachment #8678907 - Flags: review?(ekr)
Comment on attachment 8678916 [details] [diff] [review]
lib/base clang-format

this is clang-format applied to lib/base (with minor manual fixes) on top of the fix array patch [1]

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8678873
Attachment #8678916 - Flags: review?(ekr)
Attachment #8678907 - Flags: review?(ekr) → review?(rrelyea)
Attachment #8678916 - Flags: review?(ekr) → review?(rrelyea)

Comment 4

3 years ago
Comment on attachment 8678916 [details] [diff] [review]
lib/base clang-format

Review of attachment 8678916 [details] [diff] [review]:
-----------------------------------------------------------------

r+ minor nit, we are now splitting lines, we should remove comments explaining why the lines weren't split. OK to make the comment removal a separate patch after this one is checked in.

What I assumed:
1) the changes were automatically generated by clang, and clang only made formatting, not refactoring changes.
2) I spot checked that assumption in a number of places.
3) I reviewed the clang formatting to make sure the result was generally readable. Ugly is in the eye of the beholder, so I did not comment on 'ugly' changes if they were consistent were readable.
4) The only issue I saw turned out to be a spinter reformating issue (splinter put a newline between type, and quanity) in two macros, but clang had the correct output.

::: lib/base/base.h
@@ +371,5 @@
>  
> +/* The following line exceeds 72 characters, but emacs screws up if I split it.
> + */
> +#define nss_ZNEWARRAY(arenaOpt, type, quantity)                                \
> +    ((type *)nss_ZAlloc((arenaOpt), sizeof(type) * (quantity)))

This looked wrong on splinter, but looking at the actual line, the clang output is fine.

Since we are now breaking up the line, we should remove the comment. I presume modern emacs can handle the split line just fine.

@@ +435,5 @@
>  #else /* DEBUG */
> +/* The following line exceeds 72 characters, but emacs screws up if I split it.
> + */
> +#define nssArena_VERIFYPOINTER(p)                                              \
> +    (((NSSArena *)NULL == (p)) ? PR_FAILURE : PR_SUCCESS)

If we split this, we can probably dispense with the comment.
Attachment #8678916 - Flags: review?(rrelyea) → review+

Comment 5

3 years ago
Comment on attachment 8678907 [details] [diff] [review]
protection

Review of attachment 8678907 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rrelyea
yes, good table to tell clang to have hands off.
Attachment #8678907 - Flags: review?(rrelyea) → review+
Created attachment 8691455 [details] [diff] [review]
LibBaseCommentRemoval.patch

removing comments about line splitting that are not valid anymore
Thanks for the review Bob.

Martin can you land this, or do we want more reviews for the clang-formatting? It's r+ by Bob and the additional comment removal patch is only removing the comments mentioned in comment 4.
Flags: needinfo?(martin.thomson)
I squashed all the commits:
https://hg.mozilla.org/projects/nss/rev/d5dd1249b522
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(martin.thomson)
Resolution: --- → FIXED
Target Milestone: --- → 3.22
You need to log in before you can comment on or make changes to this bug.