654436 - PR_ARRAY_SIZE macro: number of elements in array: sizeof(a)/sizeof(a[0])

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P3)

Description

[Konstantin Andreev]

Attachment: NSPR: 62 times of counting of number of elements in array.

The number of elements in array is counted very often: 123 times in the NSS and 62 times in the NSPR sources (see attachments).

When there is no a standard macro, you have a dilemma: write your own macro or just inline [sizeof(a)/sizeof(*a)].

Mozilla is a big application, with many people working on different parts independently. People, who choose to write a macro, start to define similar, but different names.

   NSS:  NSS_CKCAPI_ARRAY_SIZE NSS_CKMK_ARRAY_SIZE NUM_ELEM NUM_SUITEINFOS
   NSPR: countof
   JS:   JS_ARRAY_LENGTH

Other people ought to encumber the code by continuous [sizeof(longUnpleasantUnreadableName)/sizeof(*longUnpleasantUnreadableName)].

Let define a "standard" macro at the lowest possible level, and let it be adopted by NSPR, NSS, ...

Comment 1

[Konstantin Andreev]

Attachment: NSS: 123 times of counting of number of elements in array.

Comment 2

[Konstantin Andreev]

Attachment: PR_ARR_SIZE macro added

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The rest of the Mozilla client code uses NS_ARRAY_LENGTH, so I'd suggest making this PR_ARRAY_LENGTH to match.

Comment 4

[Wan-Teh Chang]

Comment on attachment 529709 [details] [diff] [review]
PR_ARR_SIZE macro added

Thank you for the patch.  I have some suggested changes below.

>+/** Number of elements in array.
>+ */

NSPR doesn't use a documentation generator that recognizes the
/** convention.  So please use the normal /*.

>+#define PR_ARR_SIZE(a) (sizeof(a)/sizeof(*(a)))

The macro should be named PR_ARRAY_SIZE.  It is not necessary to
save two characters.

Use (a)[0] instead of *(a).

Comment: please do NOT write a patch to change existing code to
use the PR_ARRAY_SIZE macro.

Comment 5

[Wan-Teh Chang]

(In reply to comment #3)
> The rest of the Mozilla client code uses NS_ARRAY_LENGTH, so I'd suggest making
> this PR_ARRAY_LENGTH to match.

Thank you for the suggestion.  "array size" is also common.  Windows has an
"ARRAYSIZE" macro.  See also
http://www.boost.org/doc/libs/1_46_1/libs/preprocessor/doc/ref/array_size.html

Comment 6

[Konstantin Andreev]

Attachment: PR_ARRAY_SIZE macro added

(In reply to comment #4)
> Comment: please do NOT write a patch to change existing code to use the PR_ARRAY_SIZE macro.

Wan-Teh, what is Mozilla policy for the code refactoring ?

I have no patch for this particular case (I have for others), but a plenty of spots in NSS cry out for changes/enhancements insistently.

One of the bad consequences of not addressing these spots in time is: the new code inherits and multiplies annoying discouraged techniques.

Conversely, consistent refactoring gradually improves the overall quality of the code.

Comment 7

[Wan-Teh Chang]

Comment on attachment 529946 [details] [diff] [review]
PR_ARRAY_SIZE macro added

r=wtc.

Please define this macro earlier in the prtypes.h file, after
the PR_ABS macro, imitating the format of existing macro
definitions and their comments.

Re: comment 6: You can write a patch to use PR_ARRAY_SIZE.  The
reason I recommended against it is that it adds noise the the
diffs.  (Some people read diffs to track down a regression or
assess the risk of an upgrade.)  But we should have some NSPR
code that uses the new macro.

Comment 8

[Konstantin Andreev]

Attachment: PR_ARRAY_SIZE macro added

According to comment 7: moved after PR_ABS; uses NSPR style comments.

Comment 9

[Wan-Teh Chang]

Attachment: PR_ARRAY_SIZE macro added (as checked in)

I edited attachment 548424 [details] [diff] [review], replaced the 'countof' macro
in pr/tests/sprintf.c with PR_ARRAY_SIZE, and checked in
the patch on the NSPR trunk (NSPR 4.8.9).

Checking in pr/include/prtypes.h;
/cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v  <--  prtypes.h
new revision: 3.45; previous revision: 3.44
done
Checking in pr/tests/sprintf.c;
/cvsroot/mozilla/nsprpub/pr/tests/sprintf.c,v  <--  sprintf.c
new revision: 3.8; previous revision: 3.7
done