superreview requested: [Bug 868678] Rename the |len| field of SECItemArray : [Attachment 745439] Rename |len| to |nitems|

[bugzill...@mozilla.org]

Wan-Teh Chang <w...@google.com> has asked Kai Engert (:kaie) <ka...@kuix.de> for
superreview:
Bug 868678: Rename the |len| field of SECItemArray
https://bugzilla.mozilla.org/show_bug.cgi?id=868678

Attachment 745439: Rename |len| to |nitems|
https://bugzilla.mozilla.org/attachment.cgi?id=745439&action=edit


------- Additional Comments from Wan-Teh Chang <w...@google.com>
The SECItemArray structure is defined as follows:

typedef struct SECItemArrayStr SECItemArray;

struct SECItemArrayStr {
SECItem *items;
unsigned int len;
};

The name of the |len| field is confusing.

1. It could be misinterpreted to mean the length in bytes.

2. It could be confused with the |len| field of the SECItem structure.

I propose that we rename the |len| field of SECItemArray. This patch
renames |len| to |nitems|, but there are other possibilities:

struct SECItemArrayStr {
SECItem *items;
unsigned int nitems;
};

struct SECItemArrayStr {
SECItem *items;
unsigned int count;
};

struct SECItemArrayStr {
SECItem *items;
unsigned int size;
};

After writing this patch, I noticed a problem with |nitems|: it looks
like |items|.

Also, I wonder if renaming the |items| field to |elems| may result
in slightly more readable code. For example, compare

/* Use the array's first item only (single stapling) */
len = 1 + ss->certStatusArray->items[0].len + 3;

with

/* Use the array's first item only (single stapling) */
len = 1 + ss->certStatusArray->elems[0].len + 3;

[bugzill...@mozilla.org]

Wan-Teh Chang <w...@google.com> has asked Kai Engert (:kaie) <ka...@kuix.de> for
superreview:
Bug 868678: Rename the |len| field of SECItemArray
https://bugzilla.mozilla.org/show_bug.cgi?id=868678

Attachment 745446: Make sure SECITEM_AllocArray sets array->items and
array->len to 0 on failure
https://bugzilla.mozilla.org/attachment.cgi?id=745446&action=edit

------- Additional Comments from Wan-Teh Chang <w...@google.com>

Brian: thank you for your opinions on the renaming patch. I'll
think more about it.

This patch fixes the SECITEM_AllocArray problem only. I assume we
want SECITEM_AllocArray to match what SECITEM_AllocItem does on
failure.

[bugzill...@mozilla.org]

Wan-Teh Chang <w...@google.com> has canceled Wan-Teh Chang <w...@google.com>'s
request for superreview:

Bug 868678: Rename the |len| field of SECItemArray
https://bugzilla.mozilla.org/show_bug.cgi?id=868678

Attachment 745446: Make sure SECITEM_AllocArray sets array->items and
array->len to 0 on failure
https://bugzilla.mozilla.org/attachment.cgi?id=745446&action=edit


------- Additional Comments from Wan-Teh Chang <w...@google.com>

Brian: I added the error checking you suggested to SECITEM_AllocArray.
I kept the assertion because I found a SECITEM_AllocArray call in
ssl3con.c that doesn't check the return value.

[bugzill...@mozilla.org]

Wan-Teh Chang <w...@google.com> has asked Kai Engert (:kaie) <ka...@kuix.de> for

superreview:
Bug 868678: Rename the |len| field of SECItemArray
https://bugzilla.mozilla.org/show_bug.cgi?id=868678

Attachment 745471: Make sure SECITEM_AllocArray sets array->items and
array->len to 0 on failure, v2
https://bugzilla.mozilla.org/attachment.cgi?id=745471&action=edit

[bugzill...@mozilla.org]

Kai Engert (:kaie) <ka...@kuix.de> has canceled Wan-Teh Chang <w...@google.com>'s
request for superreview:

Bug 868678: Rename the |len| field of SECItemArray
https://bugzilla.mozilla.org/show_bug.cgi?id=868678

Attachment 745439: Rename |len| to |nitems|
https://bugzilla.mozilla.org/attachment.cgi?id=745439&action=edit

------- Additional Comments from Kai Engert (:kaie) <ka...@kuix.de>
r=kaie because of the fixes you described in comment 1.

I don't mind renaming the len attribute.

[bugzill...@mozilla.org]

Kai Engert (:kaie) <ka...@kuix.de> has granted Wan-Teh Chang <w...@google.com>'s

request for superreview:
Bug 868678: Rename the |len| field of SECItemArray
https://bugzilla.mozilla.org/show_bug.cgi?id=868678

Attachment 745471: Make sure SECITEM_AllocArray sets array->items and
array->len to 0 on failure, v2
https://bugzilla.mozilla.org/attachment.cgi?id=745471&action=edit