Last Comment Bug 658405 - Use NS_ARRAY_LENGTH as possible in widget/src
: Use NS_ARRAY_LENGTH as possible in widget/src
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: mozilla6
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-19 15:45 PDT by Hiroyuki Ikezoe (:hiro)
Modified: 2011-05-22 05:52 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
The fix (3.63 KB, patch)
2011-05-19 15:46 PDT, Hiroyuki Ikezoe (:hiro)
karlt: review-
Details | Diff | Review
The fix for qt part (3.63 KB, patch)
2011-05-19 15:48 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Review
Revised patch (8.04 KB, patch)
2011-05-19 20:58 PDT, Hiroyuki Ikezoe (:hiro)
karlt: review+
Details | Diff | Review

Description Hiroyuki Ikezoe (:hiro) 2011-05-19 15:45:11 PDT
Use NS_ARRAY_LENGTH instead.
Comment 1 Hiroyuki Ikezoe (:hiro) 2011-05-19 15:46:04 PDT
Created attachment 533826 [details] [diff] [review]
The fix
Comment 2 Hiroyuki Ikezoe (:hiro) 2011-05-19 15:48:28 PDT
Created attachment 533827 [details] [diff] [review]
The fix for qt part

I don't like copy-n-paste codes.
Comment 3 Karl Tomlinson (ni?:karlt) 2011-05-19 16:11:40 PDT
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) 2011-05-19 17:08:18 PDT
(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) 2011-05-19 17:12:54 PDT
(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) 2011-05-19 17:17:31 PDT
(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 (ni?:karlt) 2011-05-19 17:47:15 PDT
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) 2011-05-19 17:57:16 PDT
(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) 2011-05-19 20:58:00 PDT
Created attachment 533879 [details] [diff] [review]
Revised patch

The patch also includes qt part fix.
Comment 10 Dão Gottwald [:dao] 2011-05-22 05:52:31 PDT
http://hg.mozilla.org/mozilla-central/rev/e65b514ef374

Note You need to log in before you can comment on or make changes to this bug.