1118245 - Apply uniform style across NSS and NSPR

Status: ASSIGNED

(NSS :: Libraries, defect)

Description

[Richard Barnes [:rbarnes]]

The inconsistent style of NSS makes editing difficult and impedes contribution.  We should agree on a uniform style and apply it across the code base.

In some earlier bugs (e.g., Bug 1064670) we had agreed to follow the Google C++ Style Guide for C++ files in NSS.  
https://code.google.com/p/google-styleguide/

It may be more expedient to use clang-format, since it seems a little better adapted to C (vs. C++) and has an auto-formatting tool.
http://clang.llvm.org/docs/ClangFormat.html

I don't really care.  I just don't want to have to decide whether it's tabs or spaces any more.

Comment 1

[Richard Barnes [:rbarnes]]

Patch to big for Bugzilla, posted to Rietveld.
https://codereview.appspot.com/201830043/

Comment 2

[Wan-Teh Chang]

I support this effort, and I can review (at least do a quick visual
inspection of) the reformatting patches.

I am willing to switch to a style that's quite different from the
current prevalent NSS style. It seems beneficial to just adopt the
Mozilla formatting style or Google formatting style.

A quick perusal of
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code
shows that the Mozilla formatting style (which isn't fully documented
in that page) seems closer to the current prevalent NSS formatting
style than the Google formatting style is.

The formatting style should be tailored for C code. This means the
'*' should be placed next to the variable as opposed to the type.
I am willing to not insist on this, but I hope clang-format can
deal with this.

Comment 3

[Ryan Sleevi]

If necessary, we can always create a custom style format ( http://clang.llvm.org/docs/ClangFormatStyleOptions.html )

So we could say

BasedOnStyle: Mozilla
PointerAlignment: PAS_Right

(or any other number of tweaks needed)

Comment 4

[Kai Engert (:KaiE:)]

We must use clang-format version 3.7 (unreleased), because it supports the protection boundaries and it doesn't seem to destroy inline assembler blocks (as 3.5 does).

Comment 5

[Kai Engert (:KaiE:)]

Arguments for using a style based on the Mozilla style (not Google):

(a)
NSS is tightly associated with the Mozilla project,
it's hosted on Mozilla infrastructure.

(b)
Google style:
  if (condition) action;

Mozilla style:
  if (condition)
    action;

The Mozilla style allows breakpoints on actions, and it's what apparently
most of NSS currently uses.

(c)
Using Mozilla style apparently results in less changes when reformatting NSS
(which IMHO is a good thing).


Argumens for introducing "yet another style:

(d)
Wan-Teh has already stated he wants
  PointerAlignment: PAS_Right
because NSS is C, not C++.

(e)
Bugs in the Mozilla style, we want:
  AlwaysBreakAfterDefinitionReturnType: true
  BreakBeforeBraces: Linux

Comment 6

[Kai Engert (:KaiE:)]

Attachment: _clang-format (based on Mozilla, with BreakBeforeBraces: Linux, AlwaysBreakAfterDefinitionReturnType: true, PointerAlignment: Right)

Place this style information file into the directory from where you execute clang-format, and it will automatically be loaded.

Is this a style that we all can agree to use?

Any suggestions for changes that haven't been discussed yet - or where I misunderstood the majority opinion?

Comment 7

[Kai Engert (:KaiE:)]

To get us started, I will attach sample patches which reformat the directory lib/util, certdb and certhigh.

They depend on the protection patches from bug 1137496.

Reviewing those patches should tell us, if everyone can live with the resulting format, or if further tweaks to the style are necessary.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh (using style from attachment 8570479)

Comment 9

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh (using style from attachment 8570479) (not showing whitespace changes)

Comment 10

[Eric Rescorla (:ekr)]

I would prefer to use Google style because the tooling is
much better, including the Google .el style file. Also, it's
much better documented.

It's probably worth distinguishing between two things:

1. Google (or Mozilla style)
2. clang-format's interpretation of those styles.

In many cases, the style in question permits multiple options
and the formatter necessarily picks one or the other. I don't
see any problem with saying "we're using style X as encoded
by clang-format with the following arguments" as long as we
have good reasons.

FWIW, I also would be happy saying "we're using style X
except for this exception for this reason", if we need to,
but the examples below don't seem to require that.


(In reply to Kai Engert (:kaie) from comment #5)
> Arguments for using a style based on the Mozilla style (not Google):
> 
> (a)
> NSS is tightly associated with the Mozilla project,
> it's hosted on Mozilla infrastructure.

I don't think this makes much of a difference. Frankly, Mozilla style
is so inconsistent that conformance with the rest of the code seems
not very important.


> (b)
> Google style:
>   if (condition) action;

Based on reading the style guide, this seems to be permitted
but not required:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals

"Short conditional statements may be written on one line if this enhances readability. You may use this only when the line is brief and the statement does not use the else clause."

We certainly should be able to convince clang-format to break
the line.


> Mozilla style:
>   if (condition)
>     action;
> 
> The Mozilla style allows breakpoints on actions, and it's what apparently
> most of NSS currently uses.
> 
> (c)
> Using Mozilla style apparently results in less changes when reformatting NSS
> (which IMHO is a good thing).

Given the number of changes, this seems like small beer.



> Argumens for introducing "yet another style:
> 
> (d)
> Wan-Teh has already stated he wants
>   PointerAlignment: PAS_Right
> because NSS is C, not C++.

This also appears to be consistent with Google style:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Formatting

"When declaring a pointer variable or argument, you may place the asterisk adjacent to either the type or to the variable name:"

Again we should have no trouble making clang-format do this.


> (e)
> Bugs in the Mozilla style, we want:
>   AlwaysBreakAfterDefinitionReturnType: true

I would actually prefer not to do this. I'm willing to defer to the
group if the consensus is that we do so, but I can't remember if
we reached that consensus.


>   BreakBeforeBraces: Linux

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 8570479 [details]
_clang-format (based on Mozilla, with BreakBeforeBraces: Linux, AlwaysBreakAfterDefinitionReturnType: true, PointerAlignment: Right)

renaming this attachment, as we might want to attach alternative ones

Comment 12

[Kai Engert (:KaiE:)]

Attachment: _clang-format v2 (based on Google, with PointerAlignment: Right, AllowShortIfStatementsOnASingleLine: false)

Comment 13

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh v2 (using style from attachment 8570499)

Comment 14

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh v2 (using style from attachment 8570499) (not showing whitespace changes)

Comment 15

[Kai Engert (:KaiE:)]

Attachment: subdir-clang-format.sh

Helper script, based on work from Richard Barnes. Modified to take a single parameter, which is a directory that's to be reformatted recursively.

This way the current working directory can contain the desired _clang-format file, without having to copy it to the action directory.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Eric Rescorla (:ekr) from comment #10)
> > (a)
> > NSS is tightly associated with the Mozilla project,
> > it's hosted on Mozilla infrastructure.
> 
> I don't think this makes much of a difference. Frankly, Mozilla style
> is so inconsistent that conformance with the rest of the code seems
> not very important.

The documented Mozilla style is not inconsistent. What is inconsistent is non-reformatted 15-year old Gecko code. That should not be used as an excuse to dismiss the documented Mozilla style that, yes, all new Gecko code should be following, but we also don't have any machinery to enforce that (yet).

You'll probably quickly find code style inconsistencies in NSS if you don't run clang-format on every patch, whatever code style you end up using.

> (b)
> Google style:
>   if (condition) action;
> 
> Mozilla style:
>   if (condition)
>     action;

The documented Mozilla style is actually
if (condition) {
  action;
}

Comment 17

[Kai Engert (:KaiE:)]

(In reply to Mike Hommey [:glandium] from comment #16)
> > (b)
> > Google style:
> >   if (condition) action;
> > 
> > Mozilla style:
> >   if (condition)
> >     action;
> 
> The documented Mozilla style is actually
> if (condition) {
>   action;
> }


That would be my preference, too, in memory of gotofail, but I couldn't find a clang-format preference that would add those braces.

Do you know if it exists?

Comment 18

[Kai Engert (:KaiE:)]

EKR:
Using the Mozilla style will simplify the review prior to landing the changes.

IIUC, it has been said in our discussions that people want to review the changes prior to landing. Is this correct?


There are two options to review the patches.


Approach 1: Look at the full patches.

The full patches are similar in size. However, if you click the "diff" links to the example patches in this bug, there appear to many changes to be really readable.

However, because the Mozilla style causes less changes, the display is synced better.

For example, click on both "diff" links behind the full patches, then search for e.g. ocsp_CreateCacheItemAndConsumeCertID

Although the change is in the middle of the patch, both functions headers are shown next to each other.

Using the google style patch (v2), the functions are in completely isolated blocks.


Approach 2: Look at the "ignore whitespace patches".

The size of the Mozilla style patch is 25% smaller than the google style patch, meaning, getting all our changes reviewed will be 25% less work.


I believe that means, regardless which of the patches the reviewers want to check, it will easier to review if we use the Mozilla style and keep more of the existing formatting.

Comment 19

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Many patches to Gecko require changes to both Gecko and NSS. It would be silly to force contributors to write patches in two different styles. Gecko and NSS are both Mozilla products; NSS isn't "third party" code for Mozilla.

Also, except for whitespace issues, and except for libpkix, the NSS code mostly conforms to the Mozilla coding style anyway. Ignoring libpkix (since Mozilla doesn't use it) and fixing the whitespace (replacing tabs with spaces, in particular) would solve most of the issues people experience contributing code to NSS.

Further, going forward there are likely to be many more contributors to NSS from Mozilla than from Google since Google will hardly be using NSS anymore, as their switch to BoringSSL is almost complete.

I suggest doing the reformatting in two phases:
1. Replace tabs with spaces.
2. Reformat the code to conform to the Mozilla coding style.

If NSS is going to use a different coding style from the rest of Gecko, then that should be agreed to on dev-platform, at least.

Comment 20

[Kai Engert (:KaiE:)]

It was decided:

- use a single reformatting phase (don't do a first whitespace-only phase)

- use a style based on the Mozilla style, with adjustments that make sense for NSS

- use IndentWidth 4 to minimize the amount of changes to current NSS.

Comment 21

[Kai Engert (:KaiE:)]

Attachment: _clang-format v3

This is based on the earlier Mozilla-style-variation attachment, but changing IdentWidth to 4.

FYI: If you want to run clang-format 3.7 yourself, the _clang-format must be in a parent directory of the directory you're changing, and clang-format will find and use it.

Comment 22

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh v3 (using style from attachment 8573472)

Comment 23

[Martin Thomson [:mt:]]

Kai, why don't we just land this file in the root of the repository?  Also, does a file named .clang-format work?

Comment 24

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh v3 (using style from attachment 8573472) (not showing whitespace changes)

Comment 25

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt] from comment #23)
> Kai, why don't we just land this file in the root of the repository?  Also,
> does a file named .clang-format work?

That plan sounds good to me, once the style is finalized.

Let's see if anyone has good reasons to suggest additional changes to the style, based on the patches v3 for directory lib/certhigh that are attached.

Comment 26

[Martin Thomson [:mt:]]

Comment on attachment 8573476 [details] [diff] [review]
reformat lib/certhigh v3 (using style from attachment 8573472 [details]) (not showing whitespace changes)

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

I couldn't see anything here.

I'll note though that the line length limit is really hurts readability.  There are lots of lines that are only just over the 80 line limit and those get wrapped in an often annoying fashion.

I know that we likely have 80-column die-hards here, but just scrolling through this, even as much as 5 more columns would make this a lot more readable.

Now, ideally, clang-format would respect any wrapping that was added manually and we could *recommend* a wrap at 80 columns, then force a wrap at a slightly higher number (90 or 100).  But I believe that clang-format will re-flow a manually wrapped line if it fits within its limit.  Personally, I'd prefer 90 or 100 as the hard limit, but that means that getting 80 requires the creation of temporaries and hacks like that.

Comment 27

[Wan-Teh Chang]

Comment on attachment 8573476 [details] [diff] [review]
reformat lib/certhigh v3 (using style from attachment 8573472 [details]) (not showing whitespace changes)

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

I only reviewed the first three files. Overall I think it did a fine job.

We need to use the Rietveld code review tool for this. It does a much
better job at showing white space changes.

::: lib/certhigh/certhigh.c
@@ +37,1 @@
>       * doesn't */

We will need to manually adjust these comments.

@@ +490,3 @@
>      if ( names->numnicknames ) {
> +        names->nicknames = (char **)PORT_ArenaAlloc(arena, names->numnicknames *
> +                                                               sizeof(char *));

The way clang-format folds the PORT_ArenaAlloc call is very strange.

Comment 28

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt] from comment #26)
> I'll note though that the line length limit is really hurts readability. 
> There are lots of lines that are only just over the 80 line limit and those
> get wrapped in an often annoying fashion.

I agree.


> I know that we likely have 80-column die-hards here, but just scrolling
> through this, even as much as 5 more columns would make this a lot more
> readable.

I agree.


> Now, ideally, clang-format would respect any wrapping that was added
> manually and we could *recommend* a wrap at 80 columns, then force a wrap at
> a slightly higher number (90 or 100).  But I believe that clang-format will
> re-flow a manually wrapped line if it fits within its limit.  Personally,
> I'd prefer 90 or 100 as the hard limit, but that means that getting 80
> requires the creation of temporaries and hacks like that.


I just spend another two hours playing with clang-format.

I found that it's possible to set ColumnLimit=0
which is defined as:

  A column limit of 0 means that there is no column limit. In this case, clang-format
  will respect the input’s line breaking decisions within statements unless they contradict 
  other rules.

I think we should consider to use that.

Unfortunately, that doesn't work reliably.

When using ColumnLimit=0 my two-weeks-old clang-3.7 build changes

    if (rv != SECSuccess || (usage & (certificateUsageSSLClient |
                                      certificateUsageSSLServer |
                                      certificateUsageSSLServerWithStepUp |
                                      certificateUsageEmailSigner |
                                      certificateUsageObjectSigner |
                                      certificateUsageStatusResponder |
                                      certificateUsageSSLCA)) == 0) {

into a single very long line:

    if (rv != SECSuccess || (usage & (certificateUsageSSLClient | certificateUsageSSLServer | certificateUsageSSLServerWithStepUp | certificateUsageEmailSigner | certificateUsageObjectSigner | certificateUsageStatusResponder | certificateUsageSSLCA)) == 0) {


Unfortunately, I couldn't try a newer clang-build, because the current tip of the llvm repository doesn't build (even when restricting make ONLY_TOOLS=clang).

Comment 29

[Kai Engert (:KaiE:)]

I was able to build the tip of clang, and I confirm it STILL misbehaves to join the multiple line expression into a single very long line.

In addition, it changed

    if (!flags->leafTests.cert_rev_flags_per_method
        || !flags->leafTests.preferred_methods
        || !flags->chainTests.cert_rev_flags_per_method
        || !flags->chainTests.preferred_methods) {

into

    if (!flags->leafTests.cert_rev_flags_per_method || !flags->leafTests.preferred_methods || !flags->chainTests.cert_rev_flags_per_method || !flags->chainTests.preferred_methods) {

which I think is unfortunate.


I spent a few hours trying to enhance clang-format, and I came up with a patch.


I don't know if my patch is better, but maybe we want to have a brief look if you think it does.

Comment 30

[Kai Engert (:KaiE:)]

Attachment: clang-cw0-wrap.patch (prevent endlessly long unwrapping)

This is a patch against the current tip of llvm/clang.

Comment 31

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh v4 - (using style from attachmment 8578559) (with llvm patch from attachment 8574019)

Martin, this patch was made using ColumnLimit: 0 in the clang style, and with my experimental patch applied.

Do you like this patch to certhigh better than the previous one?

Comment 32

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh v4 - (using style from attachmment 8578559) (with llvm patch from attachment 8574019) (not showing whitespace changes)

Comment 33

[Martin Thomson [:mt:]]

Comment on attachment 8574020 [details] [diff] [review]
reformat lib/certhigh v4 - (using style from attachmment 8578559) (with llvm patch from attachment 8574019 [details] [diff] [review])

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

Yes, this works much better.  I know that this means we don't have automated enforcement of a column limit, but I think that (on balance) that affords the right degree of freedom and respect for people who have to work on NSS.

Comment 34

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #27)
> We need to use the Rietveld code review tool for this. It does a much
> better job at showing white space changes.

Where can we find that tool?


> +    /* we need to handle the case where one name has an explicit token and the
> +     * other
>       * doesn't */
> 
> We will need to manually adjust these comments.


With the experiment to use ColumnWidth=0, we won't need to manually fix it,
because it should keep the length of existing comment lines unmodified.


> @@ +490,3 @@
> >      if ( names->numnicknames ) {
> > +        names->nicknames = (char **)PORT_ArenaAlloc(arena, names->numnicknames *
> > +                                                               sizeof(char *));
> 
> The way clang-format folds the PORT_ArenaAlloc call is very strange.


Apparently it decides the start column of the first parameter is a strict left boundary.

With my experimental patch to avoid the ugly long lines (as explained in comment 28/29), it looks like this:

+        names->nicknames = (char **)PORT_ArenaAlloc(arena,
+                                                    names->numnicknames *
+                                                        sizeof(char *));

Comment 35

[Kai Engert (:KaiE:)]

Unfortunately, the behaviour of clang-format 3.7-devel isn't stable yet.

The behaviour of my build from two weeks ago, as seen in patch v3, produced:

    if (keyHashEQ && (nameHash = CERT_GetSubjectNameDigest(NULL, signerCert,
                                                           hashAlg, NULL))) {


The build of today's snapshot (without my patch), using the same settings as v3 ,
now produces:

    if (keyHashEQ &&(nameHash = CERT_GetSubjectNameDigest(NULL, signerCert,
                                                          hashAlg, NULL))) {

Note the ugly

  &&(

Comment 36

[Kai Engert (:KaiE:)]

I've reported the issues with clang-format
at http://llvm.org/bugs/show_bug.cgi?id=22827 
and http://llvm.org/bugs/show_bug.cgi?id=22828

Comment 37

[Martin Thomson [:mt:]]

rietveld has an upload script here: https://code.google.com/p/rietveld/wiki/UploadPyUsage

ekr has already created an NSS repo on codereview.appspot.com, Google's public rietveld server.  That is the default server for upload.py (I personally rename this file to rietveld.py).  Usage is simple:

$ rietveld.py --rev nss

Assuming that you have a bookmark at tip called 'nss'.

Output tells you what to do, I found the interface to be easy to understand.

Repeat reviews need to use the -i flag to identify the issue number.

Comment 38

[Eric Rescorla (:ekr)]

Comment on attachment 8574020 [details] [diff] [review]
reformat lib/certhigh v4 - (using style from attachmment 8578559) (with llvm patch from attachment 8574019 [details] [diff] [review])

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

Kai, this patch doesn't seem to apply because it is on top of a patch which
wraps some of the code in clang protections.

Comment 39

[Eric Rescorla (:ekr)]

I have fixed the conflicts and uploaded the patch to Rietveld at:
https://codereview.appspot.com/211370043

Comment 40

[Eric Rescorla (:ekr)]

Attachment: "Apply uniform style across NSS" [f=martin.thomson] (merge attempt of attachment 8570444 plus attachment 8574020)

Comment 41

[Eric Rescorla (:ekr)]

Comment on attachment 8574297 [details] [diff] [review]
"Apply uniform style across NSS" [f=martin.thomson] (merge attempt of attachment 8570444 [details] [diff] [review] plus attachment 8574020 [details] [diff] [review])

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

This version of Kai's patch should apply properly.

Comment 42

[Eric Rescorla (:ekr)]

I skimmed over this diff purely to see how it looks (i.e., not a review)
and I can generally live with this style. I found a few places that aren't
totally awesome, but I think we could either fix those manually or of
they are systematic, try to force clang-format to adjust them.

I noticed two potentially systematic things:
1. clang-format unaligns initialization blocks. I.e.,

    char *    buf	= 0;
    char *    tmpbuf	= 0;
    SECItem * cn	= 0;
    SECItem * email	= 0;

becomes:

    char *buf = 0;
    char *tmpbuf = 0;
    SECItem *cn = 0;
    SECItem *email = 0;

Personally, I can live with this, since I find that attempts
to line things up often fall apart when you add one long
value. Also, NSS doesn't do this consistently. But I
wanted to point it out.


2. A few long expressions are formatted oddly. For instance:

	if ( (!validOnly || CERT_CheckCertValidTimes(cert, time, PR_FALSE)
	      == secCertTimeValid) &&

Became:

        /* If we already found the right cert, just return it */
        if ((!validOnly || CERT_CheckCertValidTimes(cert, time, PR_FALSE) ==
                               secCertTimeValid) &&

I would have expected secCertTimeValid to have lined up with !validOnly at
the opening of the expression. I didn't see a huge number of these so we
can probably adjust manually if required, but it might be nice to see if
we could figure out how to make clang-format not do that.

Comment 43

[Martin Thomson [:mt:]]

(In reply to Eric Rescorla (:ekr) from comment #42)
>         /* If we already found the right cert, just return it */
>         if ((!validOnly || CERT_CheckCertValidTimes(cert, time, PR_FALSE) ==
>                                secCertTimeValid) &&
> 
> I would have expected secCertTimeValid to have lined up with !validOnly at
> the opening of the expression. I didn't see a huge number of these so we
> can probably adjust manually if required, but it might be nice to see if
> we could figure out how to make clang-format not do that.

I don't know, it makes the precedence clearer.  I can live with that.  Personally, I'd put the break after the "||"

Comment 44

[Kai Engert (:KaiE:)]

Comment on attachment 8574297 [details] [diff] [review]
"Apply uniform style across NSS" [f=martin.thomson] (merge attempt of attachment 8570444 [details] [diff] [review] plus attachment 8574020 [details] [diff] [review])

EKR:

All patches in this file are patches on top of the patches from bug 1137496.

In order to apply patches here, you must first apply the respective patch from bug 1137496.

Comment 45

[Kai Engert (:KaiE:)]

Comment on attachment 8574297 [details] [diff] [review]
"Apply uniform style across NSS" [f=martin.thomson] (merge attempt of attachment 8570444 [details] [diff] [review] plus attachment 8574020 [details] [diff] [review])

I'm marking this as obsolete, because it isn't completely equivalent to the combination of attachment 8570444 [details] [diff] [review] plus attachment 8574020 [details] [diff] [review].

(Which shouldn't matter, this patch was simply for experimentation purposes in retfield. Besides the few differences related to the on/off wrappers, this patch is equivalent to attachment 8574020 [details] [diff] [review].)

Comment 46

[Kai Engert (:KaiE:)]

Attachment: _clang-format v4

This is the _clang-format file that uses
  ColumnLimit: 0

and which was used to create lib/certhigh patches:
  attachment 8574020 [details] [diff] [review]
  attachment 8574021 [details] [diff] [review]
  attachment 8574297 [details] [diff] [review]

while using the tip of llvm plus local patch from attachment 8574019 [details] [diff] [review]

Comment 47

[Kai Engert (:KaiE:)]

Comment on attachment 8574020 [details] [diff] [review]
reformat lib/certhigh v4 - (using style from attachmment 8578559) (with llvm patch from attachment 8574019 [details] [diff] [review])

Martin and EKR said they are generally happy with the way this patch looks.

Comment 48

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #35)
> Note the ugly
>   &&(

Upstream has fixed this bug. I've built the most recent tip of clang-format, combined with my avoid-long-unwrap patch. I'll attach updated patches v5 for lib/certhigh.

Comment 49

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh v5 - (on top of attachment 8570444) (using style from attachment 8578559) (with llvm patch from attachment 8574019)

Comment 50

[Kai Engert (:KaiE:)]

Attachment: reformat lib/certhigh v5 - (on top of attachment 8570444) (using style from attachment 8578559) (with llvm patch from attachment 8574019) (not showing whitespace changes)

Comment 51

[Kai Engert (:KaiE:)]

(In reply to Eric Rescorla (:ekr) from comment #42)
> 1. clang-format unaligns initialization blocks. I.e.,
> 
>     char *    tmpbuf	= 0;
>     SECItem * cn	= 0;
> 
> becomes:
> 
>     char *tmpbuf = 0;
>     SECItem *cn = 0;
> 
> Personally, I can live with this, since I find that attempts
> to line things up often fall apart when you add one long
> value. Also, NSS doesn't do this consistently. But I
> wanted to point it out.

I think it's OK, too.

I remember having seen many places in NSS, where parts of such blocks are already unaligned.

In the past I've experienced a human tendency to "minimize" patches during review, and reformatting such blocks makes the review more difficult, so it might be frequently skipped.


> 2. A few long expressions are formatted oddly. For instance:
> 
> 	if ( (!validOnly || CERT_CheckCertValidTimes(cert, time, PR_FALSE)
> 	      == secCertTimeValid) &&
> 
> Became:
> 
>         /* If we already found the right cert, just return it */
>         if ((!validOnly || CERT_CheckCertValidTimes(cert, time, PR_FALSE) ==
>                                secCertTimeValid) &&
> 
> I would have expected secCertTimeValid to have lined up with !validOnly at
> the opening of the expression. I didn't see a huge number of these so we
> can probably adjust manually if required, but it might be nice to see if
> we could figure out how to make clang-format not do that.


As a test, I changed ContinuationIndentWidth from 4 to 0, which produces:

        /* If we already found the right cert, just return it */
        if ((!validOnly || CERT_CheckCertValidTimes(cert, time, PR_FALSE) ==
                           secCertTimeValid) &&
            (CERT_CheckKeyUsage(cert, requiredKeyUsage) == SECSuccess) &&
            (cert->nsCertType & requiredCertType) &&
            CERT_IsUserCert(cert)) {
            return (cert);
        }

Apparently it wants to align the continued elements with the parts that belong to it from the previous line.

Do you like "ContinuationIndentWidth: 0" better?

Comment 52

[Kai Engert (:KaiE:)]

I compared the result of v5 with the full results of ContinuationIndentWidth: 0.

I like v5 (c-width: 4) better.

v5 does this:

    if (minimumSecondsToNextFetchAttempt < 
            OCSP_Global.minimumSecondsToNextFetchAttempt ||
        maximumSecondsToNextFetchAttempt <
             OCSP_Global.maximumSecondsToNextFetchAttempt) {

c-width: 4 does this:

    if (minimumSecondsToNextFetchAttempt < 
        OCSP_Global.minimumSecondsToNextFetchAttempt ||
        maximumSecondsToNextFetchAttempt <
            OCSP_Global.maximumSecondsToNextFetchAttempt) {

Comment 53

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #52)
> c-width: 4 does this:

Sorry, that should have said:

c-width: 0 does this: (v5 has 4)


> 
>     if (minimumSecondsToNextFetchAttempt < 
>         OCSP_Global.minimumSecondsToNextFetchAttempt ||
>         maximumSecondsToNextFetchAttempt <
>             OCSP_Global.maximumSecondsToNextFetchAttempt) {

Comment 54

[Martin Thomson [:mt:]]

ContinuationIndentWidth: 5 looks right to me (in v5).

Comment 55

[Kai Engert (:KaiE:)]

(In reply to Martin Thomson [:mt] from comment #37)
> rietveld has an upload script here:
> https://code.google.com/p/rietveld/wiki/UploadPyUsage

I feel somewhat uncomfortable having to run a google provided script inside my local source tree, which would have access to my credentials. 

The link to that script is even plain http, and I have no idea what company appspot.com is.

Why doesn't this tool allow me to simply upload a patch file, which I can prepare myself using "hg diff" or other equivalent commands?

I would prefer if we didn't have to rely on google stuff to do our work.

Comment 56

[Eric Rescorla (:ekr)]

(In reply to Kai Engert (:kaie) from comment #55)
> (In reply to Martin Thomson [:mt] from comment #37)
> > rietveld has an upload script here:
> > https://code.google.com/p/rietveld/wiki/UploadPyUsage
> 
> I feel somewhat uncomfortable having to run a google provided script inside
> my local source tree, which would have access to my credentials. 
> 
> The link to that script is even plain http, and I have no idea what company
> appspot.com is.

https://codereview.appspot.com/static/upload.py


> Why doesn't this tool allow me to simply upload a patch file, which I can
> prepare myself using "hg diff" or other equivalent commands?

I agree this is annoying, but I note that this is also the way that
Mozilla is moving with ReviewBoard.

Comment 57

[Kai Engert (:KaiE:)]

Ok, I'll use a separate user account, to isolate the script from my usual environment.

In bug 1137496 I've agreed with Wan-Teh that we don't need array protection in directory lib/certhigh.

I'll attach a real reformat patch next.

Comment 58

[Kai Engert (:KaiE:)]

Attachment: Reformat lib/certhigh v6

Now that we seem to agree on all the general parameters of the reformat:

Let's try a full review of a full reformat of directory lib/certhigh.

In addition to this attachment, I've also uploaded to rietveld:
https://codereview.appspot.com/218010043

Comment 59

[Kai Engert (:KaiE:)]

Attachment: Reformat lib/certhigh v6 (not showing whitespace changes)

Comment 60

[Wan-Teh Chang]

Comment on attachment 8580294 [details] [diff] [review]
Reformat lib/certhigh v6

I reviewed this patch at https://codereview.appspot.com/218010043/.
Please see my detailed comments there. Here are my high-level comments:

In general the formatting is good. I found some problems.

1. Some lines are too long.

2. Some multi-line comment blocks are indented incorrectly.

3. Some line-continuation backslashes were added incorrectly.

4. Some conditional expressions were not formatted well. A short line is added
even though the previous line still has room at the end.

Comment 61

[Franziskus Kiefer [:franziskus]]

Attachment: clang-cw0-wrap.patch (prevent endlessly long unwrapping) v2

rebased clang patch

Comment 62

[Franziskus Kiefer [:franziskus]]

Attachment: Making builds deterministic

using this patch, optimised builds should be deterministic (tested only on mac)

Comment 63

[Franziskus Kiefer [:franziskus]]

Attachment: script to compare binaries

small script to compare binaries after formatting

Comment 64

[Franziskus Kiefer [:franziskus]]

Attachment: Fixing some arrays before applying style

Changing { 0, } to { 0 } such that clang-format won't mess them up.

Comment 65

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8678873 [details] [diff] [review]
Fixing some arrays before applying style

this only removes "," many arrays (mainly ASN.1) because it confuses clang-format

Comment 66

[Eric Rescorla (:ekr)]

Comment on attachment 8678873 [details] [diff] [review]
Fixing some arrays before applying style

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

Can you put this on Rietveld since it does a better job of making clear what changed?

Comment 67

[Franziskus Kiefer [:franziskus]]

(In reply to Eric Rescorla (:ekr) from comment #66)
> Comment on attachment 8678873 [details] [diff] [review]
> Fixing some arrays before applying style
> 
> Review of attachment 8678873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you put this on Rietveld since it does a better job of making clear what
> changed?

patch is now here [1] as well

[1] https://codereview.appspot.com/271180045/

Comment 68

[Eric Rescorla (:ekr)]

Comments in Rietveld

Comment 69

[Franziskus Kiefer [:franziskus]]

updated the patches at https://codereview.appspot.com/271180045/

noticed I missed a couple of spots and also did non-number arrays now.

Comment 70

[Eric Rescorla (:ekr)]

Reviewed.

Comment 71

[Eric Rescorla (:ekr)]

LGTM. Landed as https://hg.mozilla.org/projects/nss/rev/1f9d09f9034c

Comment 72

[Kai Engert (:KaiE:)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #62)
> using this patch, optimised builds should be deterministic (tested only on
> mac)

> printf '0000000000' | dd conv=notrunc of=$@ bs=1 seek=$$((24))

What does this do?

Comment 73

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #60)
> I reviewed this patch at https://codereview.appspot.com/218010043/.
> Please see my detailed comments there. Here are my high-level comments:
> 
> 3. Some line-continuation backslashes were added incorrectly.

I don't see mistakes in the patch. The display of that review tool is misleading.

clang-format added more spaces to align the \ chars at the end of the lines. The review tool wraps the \ chars into the next line, but only in the display. If you look at the line numbers the review tool displays, you can see the \ still belongs to the same line. For example you complained about lib/certhigh/certvfy.c:293

(I personally find this external review tool confusing.)

Comment 74

[Martin Thomson [:mt:]]

Comment on attachment 8678867 [details] [diff] [review]
Making builds deterministic

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

::: coreconf/rules.mk
@@ +264,5 @@
>  else
>  	$(AR) $(OBJS)
>  endif
>  	$(RANLIB) $@
> +	printf '0000000000' | dd conv=notrunc of=$@ bs=1 seek=$$((24))

$$((24)) is just 24.  So the point of this is to seek to the 24th byte and overwrite it with a fixed value?  Why 0x2020202020202020 ?  This seems pretty fragile and dependent on all sorts of things out of our control.  I don't mind fixing the build time for objects, but I'd prefer to have a binary comparison function that ignored the bytes in question.

For the record, I think that this would probably be better:

dd if=/dev/null of=$@ bs=8 seek=3 count=1 conv=notrunc

But only in the context of doing a diff between two builds.

Comment 75

[Mike Hommey [:glandium]]

At this point, building NSS should be deterministic already, except for the .chk signatures. (and except on windows, where the linker stores the build time in the binary, but that's a toolchain thing that, if you want determinism, you should fix with a wrapper on the toolchain, not in every single software build system). IOW, I don't think this dd thing is doing anything useful at this point.

Comment 76

[Franziskus Kiefer [:franziskus]]

(In reply to Martin Thomson [:mt:] from comment #74)
> Comment on attachment 8678867 [details] [diff] [review]
> Making builds deterministic
> 
> Review of attachment 8678867 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: coreconf/rules.mk
> @@ +264,5 @@
> >  else
> >  	$(AR) $(OBJS)
> >  endif
> >  	$(RANLIB) $@
> > +	printf '0000000000' | dd conv=notrunc of=$@ bs=1 seek=$$((24))
> 
> $$((24)) is just 24.  So the point of this is to seek to the 24th byte and
> overwrite it with a fixed value?  Why 0x2020202020202020 ?  This seems
> pretty fragile and dependent on all sorts of things out of our control.  I
> don't mind fixing the build time for objects, but I'd prefer to have a
> binary comparison function that ignored the bytes in question.
> 
> For the record, I think that this would probably be better:
> 
> dd if=/dev/null of=$@ bs=8 seek=3 count=1 conv=notrunc
> 
> But only in the context of doing a diff between two builds.

yep, that looks cleaner. Ignoring the time stamps in a diff would certainly be cleaner, but also a bit more work. I did this only for the clang-format stuff and didn't want to spend too much time on it. The value I'm using to overwrite only has to be deterministic, doesn't really matter what it is.

(In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #75)
> At this point, building NSS should be deterministic already, except for the
> .chk signatures. (and except on windows, where the linker stores the build
> time in the binary, but that's a toolchain thing that, if you want
> determinism, you should fix with a wrapper on the toolchain, not in every
> single software build system). IOW, I don't think this dd thing is doing
> anything useful at this point.

If we want deterministic builds in general we should probably invest a bit more in it to make it robust. Currently Mac builds are not deterministic because of the timestamps (that's what I'm overwriting with dd here). On Linux that's not necessary, only the touch on object files is for a naive diff. Not sure about Windows.

Comment 77

[Mike Hommey [:glandium]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #76)
> If we want deterministic builds in general we should probably invest a bit
> more in it to make it robust. Currently Mac builds are not deterministic
> because of the timestamps (that's what I'm overwriting with dd here). On
> Linux that's not necessary, only the touch on object files is for a naive
> diff. Not sure about Windows.

Is mac's linker puts a timestamp in dylibs/executables, it's something to fix with a wrapper, not by changing every build system out there.

Comment 78

[Kai Engert (:KaiE:)]

Franziskus, please suggest which patches I should look at first.

Comment 79

[Franziskus Kiefer [:franziskus]]

Attachment: clang-format-update

I noticed that the newest version of clang-format sorts includes by default (or maybe it's a bug again?). But we should make sure we don't do that unless we agree that we want to sort includes.

This patch adds SortIncludes false to .clang-format.

Comment 80

[Kai Engert (:KaiE:)]

Comment on attachment 8723494 [details] [diff] [review]
clang-format-update

I agree, we shouldn't sort includes for now, r=kaie

Comment 81

[Kai Engert (:KaiE:)]

Comment on attachment 8723494 [details] [diff] [review]
clang-format-update

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

Comment 82

[Kai Engert (:KaiE:)]

It seems the following subdirectories of lib are still pending:
- jar (still waiting for me)
- libpkix (we probably don't want to touch that)
- nss
- pk11wrap
- pkcs12
- pkcs7
- pki
- smime
- sqlite (external, should be kept as is)
- sysinit
- util
- zlib (external, should probably be kept as is)

IMHO the most interesting parts to get fixed next are:
- nss + sysinit (they are small)
- util

Comment 83

[Franziskus Kiefer [:franziskus]]

clang format on pk11wrap landed as https://hg.mozilla.org/projects/nss/rev/857cc70386e76639b49a95dc68c300735bc06a3d
review at https://nss-dev.phacility.com/D124

Comment 84

[Franziskus Kiefer [:franziskus]]

clang format on pkcs12 landed as https://hg.mozilla.org/projects/nss/rev/48fe9657a2034bc52638ec793ff36cdc44b53891
review at https://nss-review.dev.mozaws.net/D3

Comment 85

[Sylvestre Ledru [:Sylvestre]]

Do you need help for this?

FYI, in bug 1274087, we are doing similar work for Firefox. We are also fixing upstream issues.

Comment 86

[Franziskus Kiefer [:franziskus]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #85)
> Do you need help for this?
> 
> FYI, in bug 1274087, we are doing similar work for Firefox. We are also
> fixing upstream issues.

Thanks for the offer, but we're nearly done. I'm doing the remaining bits this week (4 folders are left). Good to see that this is happening for FF code as well. Is that a one off effort? We ended up having an actual clang-format test suite that ensures that new code doesn't introduce badly formatted code.

Comment 87

[Sylvestre Ledru [:Sylvestre]]

> Is that a one off effort?
Nope, my hope is that it is integrated as part of the dev workflow.

Comment 88

[Franziskus Kiefer [:franziskus]]

clang format on pki, pkcs7, and jar landed as https://hg.mozilla.org/projects/nss/rev/b11a63cc651e65d6452bcf679b4fec8c79e49c05

Comment 89

[Franziskus Kiefer [:franziskus]]

clang format on smime landed as https://hg.mozilla.org/projects/nss/rev/fb4537ffb64f2a9cb9df8a11b326f308e07c8eba

Comment 90

[Franziskus Kiefer [:franziskus]]

clang format on fuzz landed as https://hg.mozilla.org/projects/nss/rev/04c5c3fe582cacea3527f24d0ff99c252c99525e

Comment 91

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/205898c824cdd4fa39bdb5d8d0a24f3ddc24ad5f
Bug 1118245 - use blacklist or clang-format instead of white list, r=ttaubert

Comment 92

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/159f96115053bf495a13660ce43ec984e5ff826f
Bug 1118245 - clang-format CI follow-up, r=ttaubert

Comment 93

[Kai Engert (:KaiE:)]

I suggest to do that for NSPR, too.