Closed Bug 1227624 Opened 5 years ago Closed 5 years ago

NSS clang-format: lib/certcb

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox45 affected)

RESOLVED FIXED
Tracking Status
firefox45 --- affected

People

(Reporter: franziskus, Assigned: franziskus)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

Applying clang-format to lib/certdb.
Attachment #8691867 - Attachment description: lib/certdb clang-format → lib/certdb clang-format protection
Attached patch lib/certdb clang-format (obsolete) — Splinter Review
here another patch with clang format and protection (first patch) for lib/certdb. Who ever has time for this can get to review it, otherwise just cancel the needinfo.

For better context, both patches are also uploaded here
protection: https://codereview.appspot.com/275560043/
clang-format: https://codereview.appspot.com/275560044/
Flags: needinfo?(wtc)
Flags: needinfo?(rrelyea)
Flags: needinfo?(martin.thomson)
Flags: needinfo?(kaie)
I updated both patches
r+ by wtc on Rietval (https://codereview.appspot.com/275560043/)
Attachment #8691867 - Attachment is obsolete: true
Attachment #8694192 - Flags: review+
Comment on attachment 8694192 [details] [diff] [review]
lib/certdb clang-format protection

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

r=wtc. Please fix the two nits I noted.

::: lib/certdb/alg1485.c
@@ +27,4 @@
>  /* Add new entries to this table, and maybe to function ParseRFC1485AVA */
>  static const NameToKind name2kinds[] = {
>  /* IANA registered type names
>   * (See: http://www.iana.org/assignments/ldap-parameters) 

Nit: please delete the space at the end of this line
(visible in the code review tool).

::: lib/certdb/genname.c
@@ +1585,1 @@
>  #define ANSSI_SUBJECT_DN \

Nit: I think this backslash should be moved to the end of the line
to match clang-format?
Attachment #8694192 - Flags: review+
fixed nits, r+ carries over
Attachment #8694192 - Attachment is obsolete: true
Attachment #8694651 - Flags: review+
Attachment #8694651 - Flags: review+
Flags: needinfo?(martin.thomson)
Attached patch lib/certdb clang-format (obsolete) — Splinter Review
rebased after protection changes
Attachment #8691868 - Attachment is obsolete: true
In file lib/certdb/alg1485.c, in cert_AVAOidTagToMaxLen and the functions that follow after it, it uses an indent of 2 spaces, not 4.

I wonder why that happened, because in other files it uses the correct indent of 4 spaces.

I see our format definition has ObjCBlockIndentWidth: 2

Does clang-format incorrectly assume that file alg1485.c is an object-c file ?

Does it use 4 spaces if you set ObjCBlockIndentWidth to 4 ?
Flags: needinfo?(kaie)
FWIW, (unpatched) clang-format version 3.7 correctly indents alg1485.c with 4 spaces.
thanks kai. not sure what happened to that file. This is a new patch where the indent looks correct (it's the same as the last one at https://codereview.appspot.com/275560044 if you want to check the differences).
Attachment #8696690 - Attachment is obsolete: true
Flags: needinfo?(kaie)
Comment on attachment 8696937 [details] [diff] [review]
lib/certdb clang-format

r=kaie

I skimmed through it, and it seems OK.

I found the following minor issues, but I think, if we want to fix them, they should be done in a follow-up patch.


(a)
+#define CASE(i, m)  \
+    case i:         \
+        CGET(i, m); \
+        if (!n)     \
+        goto unsupported /* fall-through */

indent the goto


(b)
+        /* If tag name is too long, we know it is an OID form that was
+     * allocated from the heap, so we can modify it in place
+     */

The first line of a multi line comment gets indented incorrectly.
There are multiple places like this.


(c)
     if (rv == SECSuccess &&
-        ((trust.sslFlags & CERTDB_USER ) ||
-         (trust.emailFlags & CERTDB_USER ) ||
-         (trust.objectSigningFlags & CERTDB_USER )) ) {
+        ((trust.sslFlags & CERTDB_USER) || (trust.emailFlags & CERTDB_USER) ||
+         (trust.objectSigningFlags & CERTDB_USER))) {


The original style was more readable, but I think the new one is acceptable and can be kept.
Flags: needinfo?(wtc)
Flags: needinfo?(rrelyea)
Flags: needinfo?(kaie)
Attachment #8696937 - Flags: review+
https://hg.mozilla.org/projects/nss/rev/3896627e2a2f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.22
You need to log in before you can comment on or make changes to this bug.