NSS clang-format: lib/softoken

RESOLVED FIXED in 3.27

Status

NSS
Libraries
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: fkiefer, Assigned: fkiefer)

Tracking

(Blocks: 1 bug)

trunk
3.27

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 8780007 [details] [diff] [review]
softoken-cf.patch

that's a pretty long one.
I checked that binaries don't change and try looks green as well

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=62ecfab952f5b70abcd422dc3343c6fdba24dfbf
Attachment #8780007 - Flags: review?(kaie)
Created attachment 8780008 [details] [diff] [review]
softoken-cf-tc.patch

adding softoken clang-format to TC
Attachment #8780008 - Flags: review?(ttaubert)

Comment 2

2 years ago
Comment on attachment 8780007 [details] [diff] [review]
softoken-cf.patch

r=kaie

I think it's OK to check this in.

I found a few rather long lines, around 120-140 chars,

Too bad clang-format apparently doesn't wrap long conditions with our configuration, but overall, I think the few long lines are bearable.

Given that we want automatic formatting for future changes, we probably cannot wrap it manually.

I found a rather long line:
  trustFlags = trust->trust->sslFlags & CERTDB_TRUSTED_CLIENT_CA ? trust->trust->sslFlags | CERTDB_TRUSTED_CA : 0;

If anyone think this is ugly, we could make a follow up commit, that creates an if else statement instead of ? :


Another long line candidate for follow up cleanup is
  if ((obj->objclass == CKO_CERTIFICATE) || (obj->objclass == CKO_PRIVATE_KEY) || (obj->objclass == CKO_PUBLIC_KEY) || (obj->objclass == CKO_SECRET_KEY)) {

Not sure what to do here. A switch inside a switch would require a double break would be ugly.
Does it make a different for clang-format, if the unnecessary braces are removed?

These lines:
  +        if ((cd->classFlags & LG_CERT) && !lg_tokenMatch(cd->sdb, &cert->certKey, LG_TOKEN_TYPE_CERT, cd->template, cd->templ_count)) {
  +        if ((cd->classFlags & LG_TRUST) && !lg_tokenMatch(cd->sdb, &cert->certKey, LG_TOKEN_TYPE_TRUST, cd->template, cd->templ_count)) {

Could potentially be simplified by splitting the condition into a double if, like
  if (cd->classFlags & LG_CERT) {
    if (!lg_tokenMatch(cd->sdb, &cert->certKey, LG_TOKEN_TYPE_CERT, cd->template, cd->templ_count)) {

We probably cannot simplify this one:
  if (!((buf[0] == (unsigned char)CERT_DB_FILE_VERSION) || (buf[0] == (unsigned char)CERT_DB_V7_FILE_VERSION))) {


But these ideas shouldn't block landing of this huge patch, they should be done in a follow-up, if it's considered worthy at all.
Attachment #8780007 - Flags: review?(kaie) → review+
Attachment #8780008 - Flags: review?(ttaubert) → review+
landed as
https://hg.mozilla.org/projects/nss/rev/5430165f67fd
https://hg.mozilla.org/projects/nss/rev/8f99d6551b71
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.27
You need to log in before you can comment on or make changes to this bug.