Open
Bug 1137496
Opened 10 years ago
Updated 1 year ago
clang-format NSS: land clang-format off/on protection wrappers
Categories
(NSS :: Libraries, defect, P5)
Tracking
(Not tracked)
NEW
People
(Reporter: KaiE, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 3 obsolete files)
35.46 KB,
patch
|
Details | Diff | Splinter Review | |
10.70 KB,
patch
|
Details | Diff | Splinter Review | |
118.10 KB,
patch
|
Details | Diff | Splinter Review | |
7.53 KB,
patch
|
Details | Diff | Splinter Review |
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.)
Reporter | ||
Comment 1•10 years ago
|
||
I suggest that as part of adding the protection boundaries, we should manually clean up the whitespace (remove tabs) in those sections?
Reporter | ||
Comment 2•10 years ago
|
||
Attachment #8570444 -
Flags: review?(wtc)
Reporter | ||
Comment 3•10 years ago
|
||
Reporter | ||
Comment 4•10 years ago
|
||
Attachment #8570451 -
Flags: review?(rrelyea)
Reporter | ||
Comment 5•10 years ago
|
||
Reporter | ||
Comment 6•10 years ago
|
||
Attachment #8570456 -
Flags: review?(ryan.sleevi)
Reporter | ||
Comment 7•10 years ago
|
||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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 10•10 years ago
|
||
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-
Reporter | ||
Comment 11•10 years ago
|
||
(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?
Reporter | ||
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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+
Reporter | ||
Comment 14•10 years ago
|
||
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
Reporter | ||
Updated•10 years ago
|
Attachment #8570450 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #8580266 -
Attachment is obsolete: true
Updated•2 years ago
|
Severity: normal → S3
Updated•1 year ago
|
Severity: S3 → S4
Priority: -- → P5
You need to log in
before you can comment on or make changes to this bug.
Description
•