658405 - Use NS_ARRAY_LENGTH as possible in widget/src

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Use NS_ARRAY_LENGTH instead.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: The fix

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: The fix for qt part

I don't like copy-n-paste codes.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 533826 [details] [diff] [review]
The fix

>-    length = NS_ARRAY_LENGTH(nsKeycodes);
>-    for (i = 0; i < length; ++i) {
>+    for (i = 0; i < nsKeycodesLength; ++i) {
>       if (nsKeycodes[i].vkCode == aKeysym) {

I actually prefer the previous style, having NS_ARRAY_LENGTH(nsKeycodes) visible where it is used, so that I don't need to know that nsKeycodesLength has been correctly initialized for nsKeycodes.  NS_ARRAY_LENGTH(nsKeycodes) involves only constants so will be optimized away by the compiler.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #3)
> Comment on attachment 533826 [details] [diff] [review] [review]
> The fix
> 
> >-    length = NS_ARRAY_LENGTH(nsKeycodes);
> >-    for (i = 0; i < length; ++i) {
> >+    for (i = 0; i < nsKeycodesLength; ++i) {
> >       if (nsKeycodes[i].vkCode == aKeysym) {http://tracking.
> 
> I actually prefer the previous style, having NS_ARRAY_LENGTH(nsKeycodes)
> visible where it is used, so that I don't need to know that nsKeycodesLength
> has been correctly initialized for nsKeycodes.  NS_ARRAY_LENGTH(nsKeycodes)
> involves only constants so will be optimized away by the compiler.

I respect your preference.  I will revise the patch later.

Just out of curiosity, which do you prefer?

a)
int i, cartItemsLength = NS_ARRAY_LENGTH(cartItems);

for (i = 0; i < cartItemsLength; i++) {
  do_something();
}

for (i = cartItemsLength; i < cartItemsLength * 2; i++) {
  do_other_thing();
}

b)

int i;
..
for (i = 0; i < NS_ARRAY_LENGTH(cartItems); i++) {
  do_something();
}

for (i = NS_ARRAY_LENGTH(cartItems); i < NS_ARRAY_LENGTH(cartItems) * 2; i++) {
  do_other_thing();
}


Actually these examples are not so good, but anyway I prefer the former when variable itself is a good name (represents itself well) rather than scattering methods and macros.

IMHO, when the name (variable name, function name, macro name and whatever its name) is appropriate, I do not care about it (in many cases, I assume it's correct. I do care about it only if there is a bug and the name does not represent what it is!).

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 533826 [details] [diff] [review] [review] [review]
> > The fix
> > 
> > >-    length = NS_ARRAY_LENGTH(nsKeycodes);
> > >-    for (i = 0; i < length; ++i) {
> > >+    for (i = 0; i < nsKeycodesLength; ++i) {
> > >       if (nsKeycodes[i].vkCode == aKeysym) {http://tracking.
> > 
> > I actually prefer the previous style, having NS_ARRAY_LENGTH(nsKeycodes)
> > visible where it is used, so that I don't need to know that nsKeycodesLength
> > has been correctly initialized for nsKeycodes.  NS_ARRAY_LENGTH(nsKeycodes)
> > involves only constants so will be optimized away by the compiler.
> 
> I respect your preference.  I will revise the patch later.

I misunderstood your comment? You mean this bug is "WONTFIX"?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #3)
> Comment on attachment 533826 [details] [diff] [review] [review]
> The fix
> 
> >-    length = NS_ARRAY_LENGTH(nsKeycodes);
> >-    for (i = 0; i < length; ++i) {
> >+    for (i = 0; i < nsKeycodesLength; ++i) {
> >       if (nsKeycodes[i].vkCode == aKeysym) {
> 
> I actually prefer the previous style, having NS_ARRAY_LENGTH(nsKeycodes)
> visible where it is used, so that I don't need to know that nsKeycodesLength
> has been correctly initialized for nsKeycodes. 

Hmm, another question.

Why can you trust NS_ARRAY_LENGTH macro rather than sizeof(some) / sizeof(some[0]) ?

Comment 7

[Karl Tomlinson (:karlt)]

Bear in mind that the changes proposed here are style changes to code that someone chose to write in a different way, and that we don't usually change code unless there is a clear advantage in doing so.  The current code has had years of testing, and it is easier to research the history of a file without digging through style changes.(In reply to comment #4)

> (In reply to comment #3)
> Just out of curiosity, which do you prefer?
> 
> a)
> int i, cartItemsLength = NS_ARRAY_LENGTH(cartItems);
> 
> for (i = 0; i < cartItemsLength; i++) {
>   do_something();
> }
> 
> for (i = cartItemsLength; i < cartItemsLength * 2; i++) {
>   do_other_thing();
> }
> 
> b)
> 
> int i;
> ..
> for (i = 0; i < NS_ARRAY_LENGTH(cartItems); i++) {
>   do_something();
> }
> 
> for (i = NS_ARRAY_LENGTH(cartItems); i < NS_ARRAY_LENGTH(cartItems) * 2;
> i++) {
>   do_other_thing();
> }

Whichever makes the code easier to read.
The compiler should generate the same code.

"NS_ARRAY_LENGTH(cartItems) * 2" is unlikely to happen.  The point of NS_ARRAY_LENGTH is that we know it is the end of the array, so going beyond that would be strange.

I don't really have a strong preference in the examples, but perhaps the former is clearer that there is multiple use of the same constant.

Note that "for (int i =," is usually more appropriate, but I wouldn't change existing code because I preferred that.

(In reply to comment #6)
> Why can you trust NS_ARRAY_LENGTH macro rather than sizeof(some) /
> sizeof(some[0]) ?

NS_ARRAY_LENGTH is equivalent to sizeof(some) / sizeof(some[0]).
It is created for this purpose, to ensure that nobody accidentally does sizeof(a)/sizeof(b[0]) and to make the code a bit shorter.

(In reply to comment #5)
> You mean this bug is "WONTFIX"?

In the sense that the array length is not calculated each time, I guess this bug is invalid, but I don't like "sizeof(nsKeycodes) / sizeof(struct nsKeyConverter)", because, to understand that, the reader needs to know that nsKeycodes is an array of nsKeyConverter, so I'd take a patch to use NS_ARRAY_LENGTH.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #7)
> I don't really have a strong preference in the examples, but perhaps the
> former is clearer that there is multiple use of the same constant.

Yeah, I just wanted to say it.

> (In reply to comment #6)
> > Why can you trust NS_ARRAY_LENGTH macro rather than sizeof(some) /
> > sizeof(some[0]) ?
> 
> NS_ARRAY_LENGTH is equivalent to sizeof(some) / sizeof(some[0]).
> It is created for this purpose, to ensure that nobody accidentally does
> sizeof(a)/sizeof(b[0]) and to make the code a bit shorter.

Unfortunately someone already did.

http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsPaperPS.cpp#45

> (In reply to comment #5)
> > You mean this bug is "WONTFIX"?
> 
> In the sense that the array length is not calculated each time, I guess this
> bug is invalid, 

Right, I misunderstood it.

> but I don't like "sizeof(nsKeycodes) / sizeof(struct
> nsKeyConverter)", because, to understand that, the reader needs to know that
> nsKeycodes is an array of nsKeyConverter, so I'd take a patch to use
> NS_ARRAY_LENGTH.

Thanks, I am changing the bug summery and will attach a new patch later.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

The patch also includes qt part fix.

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e65b514ef374