Open Bug 1118245 Opened 9 years ago Updated 4 months ago

Apply uniform style across NSS and NSPR

Categories

(NSS :: Libraries, defect, P4)

Tracking

(Not tracked)

People

(Reporter: rbarnes, Unassigned)

References

(Depends on 1 open bug)

Details

(Whiteboard: [some patches here ARE ON TOP of the patches from bug 1137496])

Attachments

(9 files, 15 obsolete files)

295 bytes, text/plain
Details
1.93 KB, text/plain
Details
547.13 KB, patch
wtc
: review-
Details | Diff | Splinter Review
75.25 KB, patch
Details | Diff | Splinter Review
804 bytes, patch
Details | Diff | Splinter Review
845 bytes, patch
Details | Diff | Splinter Review
576 bytes, application/x-sh
Details
44.28 KB, patch
Details | Diff | Splinter Review
703 bytes, patch
KaiE
: review+
Details | Diff | Splinter Review
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.
Patch to big for Bugzilla, posted to Rietveld.
https://codereview.appspot.com/201830043/
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.
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)
Depends on: 1137496
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).
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
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?
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.
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 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
Attachment #8570479 - Attachment description: _clang-format → _clang-format (based on Mozilla, with BreakBeforeBraces: Linux, AlwaysBreakAfterDefinitionReturnType: true, PointerAlignment: Right)
Attachment #8570488 - Attachment description: reformat lib/certhigh → reformat lib/certhigh (using style from attachment 8570479)
Attachment #8570489 - Attachment description: reformat lib/certhigh (not showing whitespace changes) → reformat lib/certhigh (using style from attachment 8570479) (not showing whitespace changes)
Attachment #8570499 - Attachment description: _clang-format (based on Google, with PointerAlignment: Right, AllowShortIfStatementsOnASingleLine: false) → _clang-format v2 (based on Google, with PointerAlignment: Right, AllowShortIfStatementsOnASingleLine: false)
Attached file 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.
(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;
}
(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?
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.
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.
Attachment #8570499 - Attachment is obsolete: true
Attachment #8570500 - Attachment is obsolete: true
Attachment #8570501 - Attachment is obsolete: true
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.
Attached file _clang-format v3 (obsolete) —
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.
Attachment #8570479 - Attachment is obsolete: true
Attachment #8570488 - Attachment is obsolete: true
Attachment #8570489 - Attachment is obsolete: true
Kai, why don't we just land this file in the root of the repository?  Also, does a file named .clang-format work?
(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 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 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.
(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).
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.
This is a patch against the current tip of llvm/clang.
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?
Attachment #8574020 - Flags: feedback?(martin.thomson)
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.
Attachment #8574020 - Flags: feedback?(martin.thomson) → feedback+
(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 *));
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

  &&(
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 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.
I have fixed the conflicts and uploaded the patch to Rietveld at:
https://codereview.appspot.com/211370043
Attachment #8574020 - Attachment is obsolete: true
Assignee: nobody → ekr
Status: NEW → ASSIGNED
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.
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.
(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 "||"
Whiteboard: [patches here ARE ON TOP of the patches from bug 1137496]
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 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].)
Attachment #8574297 - Attachment is obsolete: true
Attached file _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]
Attachment #8573472 - Attachment is obsolete: true
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.
Attachment #8574020 - Attachment description: experimental patch for Martin → reformat lib/certhigh v4 - (using style from attachmment 8578559) (with llvm patch from attachment 8574019)
Attachment #8574020 - Flags: feedback+
Attachment #8574020 - Attachment is obsolete: false
Attachment #8574021 - Attachment description: experimental patch for Martin (not showing whitespace changes) → reformat lib/certhigh v4 - (using style from attachmment 8578559) (with llvm patch from attachment 8574019) (not showing whitespace changes)
Attachment #8574297 - Attachment description: "Apply uniform style across NSS" [f=martin.thomson] → "Apply uniform style across NSS" [f=martin.thomson] (merge attempt of attachment 8570444 plus attachment 8574020)
Attachment #8573474 - Attachment is obsolete: true
Attachment #8573476 - Attachment is obsolete: true
Attachment #8574019 - Attachment description: clang-cw0-wrap.patch → clang-cw0-wrap.patch (prevent endlessly long unwrapping)
(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.
Attachment #8578577 - Attachment description: reformat lib/certhigh v5 - (on top of attachment 8570444) (using style from attachmment 8578559) (with llvm patch from attachment 8574019) → reformat lib/certhigh v5 - (on top of attachment 8570444) (using style from attachment 8578559) (with llvm patch from attachment 8574019)
Attachment #8578578 - Attachment description: reformat lib/certhigh v5 - (on top of attachment 8570444) (using style from attachmment 8578559) (with llvm patch from attachment 8574019) (not showing whitespace changes) → reformat lib/certhigh v5 - (on top of attachment 8570444) (using style from attachment 8578559) (with llvm patch from attachment 8574019) (not showing whitespace changes)
(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?
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) {
(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) {
ContinuationIndentWidth: 5 looks right to me (in v5).
(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.
(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.
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.
Whiteboard: [patches here ARE ON TOP of the patches from bug 1137496] → [some patches here ARE ON TOP of the patches from bug 1137496]
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
Attachment #8578577 - Attachment is obsolete: true
Attachment #8578578 - Attachment is obsolete: true
Attachment #8580294 - Flags: review?(wtc)
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.
Attachment #8580294 - Flags: review?(wtc) → review-
Depends on: 1204998
using this patch, optimised builds should be deterministic (tested only on mac)
small script to compare binaries after formatting
Changing { 0, } to { 0 } such that clang-format won't mess them up.
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
Attachment #8678873 - Flags: review?(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?
(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/
Flags: needinfo?(ekr)
Comments in Rietveld
Flags: needinfo?(ekr)
updated the patches at https://codereview.appspot.com/271180045/

noticed I missed a couple of spots and also did non-number arrays now.
Flags: needinfo?(ekr)
Reviewed.
Flags: needinfo?(ekr)
Attachment #8678873 - Flags: review?(ekr)
(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?
(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 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.
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.
(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.
(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.
Franziskus, please suggest which patches I should look at first.
Flags: needinfo?(franziskuskiefer)
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.
Flags: needinfo?(kaie)
Comment on attachment 8723494 [details] [diff] [review]
clang-format-update

I agree, we shouldn't sort includes for now, r=kaie
Flags: needinfo?(kaie)
Attachment #8723494 - Flags: review+
Depends on: 1254918
Flags: needinfo?(franziskuskiefer)
Depends on: 1293528
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
Do you need help for this?

FYI, in bug 1274087, we are doing similar work for Firefox. We are also fixing upstream issues.
(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.
> Is that a one off effort?
Nope, my hope is that it is integrated as part of the dev workflow.
I suggest to do that for NSPR, too.
Summary: Apply uniform style across NSS → Apply uniform style across NSS and NSPR
Depends on: 1375876
Severity: normal → S3
Severity: S3 → S4
Priority: -- → P4

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: ekr → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.