Open Bug 1137496 Opened 9 years ago Updated 8 months ago

clang-format NSS: land clang-format off/on protection wrappers

Categories

(NSS :: Libraries, defect, P5)

3.18

Tracking

(Not tracked)

People

(Reporter: KaiE, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

We want to reformat all of NSS (excluding third party code AAUI), using clang-format.

clang-format has a property that we don't like: it's reformats initialized arrays to have different numbers of items per line.

Because NSS contains many such arrays which have been manually aligned for readability or for the purpose of grouping, and because clang-format would destroy that manual work, we would like to add instructions to the NSS source code, that instruct clang-format to keep those sections untouched.

Because we're talking C code, this is the syntax that must be used for such blocks:

int formatted_code;
/* clang-format off */
    void    unformatted_code  ;
/* clang-format on */
void formatted_code_again;


The purpose of this bug is to attach multiple patches that add such protection, and keep the bug open as we're working on it.

We haven't decided yet if reviews for such patches can be omitted.

For now, let's assume patches need review as usual. (We might decide otherwise later in the process, if we do, we should update this bug's "status whiteboard" to indicate the current rule.)
Blocks: 1118245
I suggest that as part of adding the protection boundaries, we should manually clean up the whitespace (remove tabs) in those sections?
Comment on attachment 8570444 [details] [diff] [review]
Directory certhigh: protect arrays, replace tabs in those arrays

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

This looks fine.  I can't see any trailing whitespace in the protected areas either, which is my other concern.  One problem with using splinter for this is that it loses some of the protected areas.  I'm not going to check to see if there is actual code (or trailing WS) in those blocks.
Comment on attachment 8570444 [details] [diff] [review]
Directory certhigh: protect arrays, replace tabs in those arrays

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

::: lib/certhigh/certreq.c
@@ +41,2 @@
>      { SEC_ASN1_CONSTRUCTED | SEC_ASN1_CONTEXT_SPECIFIC | 0,
> +      offsetof(CERTCertificateRequest,attributes), 

Trailing ws.  whitespace-cleanup in emacs or equivalent is probably wise to avoid this.
Comment on attachment 8570444 [details] [diff] [review]
Directory certhigh: protect arrays, replace tabs in those arrays

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

Kai, thank you for preparing these patches.

::: lib/certhigh/certreq.c
@@ +20,3 @@
>      { SEC_ASN1_OBJECT_ID, offsetof(CERTAttribute, attrType) },
>      { SEC_ASN1_SET_OF | SEC_ASN1_XTRN, offsetof(CERTAttribute, attrValue),
> +    SEC_ASN1_SUB(SEC_AnyTemplate) },

Lines 19 and 22 are not aligned correctly.

::: lib/certhigh/certvfypkix.c
@@ +187,5 @@
>      {certUsageUserCertImport,        ekuIndexUnknown},
>      {certUsageVerifyCA,              ekuIndexUnknown},
>      {certUsageProtectedObjectSigner, ekuIndexUnknown},
>      {certUsageStatusResponder,       ekuIndexStatusResponder},
>      {certUsageAnyCA,                 ekuIndexUnknown},

I'm OK with not manually formatting this array.

::: lib/certhigh/ocsp.c
@@ +1065,4 @@
>   */
>  static const SEC_ASN1Template ocsp_OCSPRequestTemplate[] = {
>      { SEC_ASN1_SEQUENCE,
> +        0, NULL, sizeof(CERTOCSPRequest) },

I think we should document how the ASN.1 templates should be formatted.

Does clang-format do a poor job formatting the ASN.1 templates?
Attachment #8570444 - Flags: review?(wtc) → review-
Attached patch sample asn1 template reformat (obsolete) — Splinter Review
(In reply to Wan-Teh Chang from comment #10)
> >  static const SEC_ASN1Template ocsp_OCSPRequestTemplate[] = {
> >      { SEC_ASN1_SEQUENCE,
> > +        0, NULL, sizeof(CERTOCSPRequest) },
> 
> I think we should document how the ASN.1 templates should be formatted.
> 
> Does clang-format do a poor job formatting the ASN.1 templates?

This is an example of how it reformats.

Do you think we should allow clang-format to reformat all our asn1 templates?
Wan-Teh, the only arrays in lib/certhigh are:
- asn.1 templates
- the one array you mentioned which you think is fine to have unprotected.

If you were OK with reformatting the asn.1 templates, then we could declare:

  "directory lib/certhigh doesn't need any protection".

OK?
Comment on attachment 8580266 [details] [diff] [review]
sample asn1 template reformat

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

r=wtc. I think clang-format did a good job formatting the ASN.1 templates
in lib/certhigh/ocsp.c.

::: lib/certhigh/ocsp.c
@@ +1070,2 @@
>      { SEC_ASN1_OPTIONAL | SEC_ASN1_EXPLICIT |
> +          SEC_ASN1_CONSTRUCTED | SEC_ASN1_CONTEXT_SPECIFIC | 0,

I would not indent this continuation line, but this is fine by me.
Attachment #8580266 - Flags: review+
Comment on attachment 8570444 [details] [diff] [review]
Directory certhigh: protect arrays, replace tabs in those arrays

Marking patch as obsolete, as we agree that directory lib/certhigh doesn't need protection.
Attachment #8570444 - Attachment is obsolete: true
Attachment #8570450 - Attachment is obsolete: true
Attachment #8580266 - Attachment is obsolete: true
Severity: normal → S3
Severity: S3 → S4
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: