693469 - (arraylength) Implement mozilla::ArrayLength and mozilla::ArrayEnd

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

The common idioms:

#define NS_ARRAY_LENGTH(arr) (sizeof(arr) / sizeof(arr[0]))
#define NS_ARRAY_END(arr) (arr + NS_ARRAY_LENGTH(arr))

are arguably buggy in that they'll cheerfully accept a plain old pointer that's not an array and give you a probably-wrong answer.  These don't have that problem:

template<typename T, size_t N> size_t
ArrayLength(T (&arr)[N])
{
    return N;
}

template<typename T, size_t N> T*
ArrayEnd(T (&arr)[N])
{
    return arr + ArrayLength(arr);
}

What say you to adding them to mfbt/Util.h?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sounds like a good idea to me.

The only issue is that calls to ArrayLength() won't boil away in debug builds so there's a possibility of a small slowdown, but I don't think that's likely, and this way is friendlier to debuggers.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: 1 - Implement the two methods, convert most (but not all :-( ) users to them

There are a few little trickinesses to doing this:

* NS_ARRAY_LENGTH is constexpr, ArrayLength is not.

This means every use which feeds into a template parameter, or which gets used for the length of a sibling array, is pretty much untouchable.  One or two places, I could merge the two arrays into one array of structs whose fields were the individual things.  The gssapi changes are one such place.  Most places, it wasn't feasible: the length came from an enum or something, either array was actually used as an array, (most perniciously) the length fed into a static variable (implicitly potentially creating a static initializer), or (most often) the use fed into a static assert.  It'd be great to get rid of those places, too, and remove the macros entirely.  But converting compile-time checking to runtime checking will make most developers less happy.  So we're stuck with this until we get broad C++ compiler support for constexpr.

* using namespace mozilla/namespace dom = mozilla::dom ambiguates dom::Foo.

If you do this, it turns out that dom::Foo is ambiguous between dom::Foo and mozilla::mozilla::dom.  Funtimes.  I removed the namespace dom = to resolve that ambiguity, whenever such ambiguity was encountered (half a dozen times?).

* Templates can't depend on function-local types.

So using ArrayLength on that little array of a nested class is verboten, and you have to move the array out.  (See also: MXR search for "boo-urns".)

* Some uses were in htmlparser generated files.

I undid the work the global s&r did for these files.

* A couple uses were in headers.

For these I forewent using namespace mozilla and did mozilla::ArrayLength (or moved the using more locally, although I think I did this only once).

* One place, the change from raw numeric literal math (int?) to typed size_t caused a compile error.

That's why the change to parser/htmlparser/src/nsDTDUtils.h from PRUint32 to size_t for mCount.

* Some of the test files pick up the include from TestHarness.h or some other central test file.

They're supposed to be convenient, so they do that.  The individual tests still have to using their way to success, tho (EIBT).

* We have no idea what order we want our includes to be in.

I put it near the start generally (usually after <> includes but before anything else).  I was probably not fully consistent about this.

Interesting side note: apparently something conflicts between Util.h and prtypes.h.  If I included them in one order (well, at some nesting of the latter) in nsNSSIOLayer.cpp, I got a compile error from a PLArenaPool that transmogrified into a PRArenaPool.  In the other order, it compiled.  (It's something about the PR_BEGIN_EXTERN_C/PR_END_EXTERN_C "block" in prtypes.h -- before dies, after succeeds.)  My nsNSSIOLayer.i-fu failed me at figuring out the exact problem, so I punted and just put that one Util.h include in a weird place.

Aside from the obvious s&r, this was mostly manual, due to the including and usings that had to be carefully placed to work and not break stuff.  (See prtypes.h.)  So I have no idea how you want to review this.

JS_ARRAY_LENGTH and JS_ARRAY_END are a separate (and much smaller) patch, coming up.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 2 - Get rid of most JS_ARRAY_LENGTH/END uses

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 566129 [details] [diff] [review]
1 - Implement the two methods, convert most (but not all :-( ) users to them

This is pretty seriously epically heroic.  Thanks!

>diff --git a/browser/components/about/AboutRedirector.cpp b/browser/components/about/AboutRedirector.cpp
>--- a/browser/components/about/AboutRedirector.cpp
>+++ b/browser/components/about/AboutRedirector.cpp
>@@ -39,6 +39,8 @@
> 
> // See also: docshell/base/nsAboutRedirector.cpp
> 
>+#include "mozilla/Util.h"
>+
> #include "AboutRedirector.h"
> #include "nsNetUtil.h"
> #include "nsIScriptSecurityManager.h"

Vestige?

>diff --git a/browser/components/migration/src/nsIEProfileMigrator.cpp b/browser/components/migration/src/nsIEProfileMigrator.cpp
>--- a/browser/components/migration/src/nsIEProfileMigrator.cpp
>+++ b/browser/components/migration/src/nsIEProfileMigrator.cpp
>@@ -39,6 +39,8 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include "mozilla/Util.h"
>+
> #include <stdio.h>
> #include <string.h>
> #include <windows.h>
>@@ -109,6 +111,8 @@
> #define REGISTRY_IE_SEARCHURL_KEY \
>   NS_LITERAL_STRING("Software\\Microsoft\\Internet Explorer\\SearchUrl")
> 
>+using namespace mozilla;
>+
> const int sInitialCookieBufferSize = 1024; // but it can grow
> const int sUsernameLengthLimit     = 80;
> const int sHostnameLengthLimit     = 255;
>@@ -1673,7 +1677,7 @@ nsIEProfileMigrator::CopyPreferences(boo
> {
>   bool            regKeyOpen = false;
>   const regEntry  *entry,
>-                  *endEntry = gRegEntries + NS_ARRAY_LENGTH(gRegEntries);
>+                  *endEntry = gRegEntries + ArrayLength(gRegEntries);

ArrayEnd(gRegEntries)?

>diff --git a/docshell/base/nsAboutRedirector.cpp b/docshell/base/nsAboutRedirector.cpp
>--- a/docshell/base/nsAboutRedirector.cpp
>+++ b/docshell/base/nsAboutRedirector.cpp
>@@ -37,12 +37,16 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include "mozilla/Util.h"
>+
> #include "nsAboutRedirector.h"
> #include "nsNetUtil.h"
> #include "plstr.h"
> #include "nsIScriptSecurityManager.h"
> #include "nsAboutProtocolUtils.h"
> 
>+using namespace mozilla;
>+
> NS_IMPL_ISUPPORTS1(nsAboutRedirector, nsIAboutModule)
> 
> struct RedirEntry {

Vestige?

>diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp b/gfx/layers/opengl/LayerManagerOGL.cpp
>--- a/gfx/layers/opengl/LayerManagerOGL.cpp
>+++ b/gfx/layers/opengl/LayerManagerOGL.cpp
>@@ -37,6 +37,8 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include "mozilla/Util.h"
>+
> #include "mozilla/layers/PLayers.h"
> 
> #include "LayerManagerOGL.h"
>@@ -258,7 +260,7 @@ LayerManagerOGL::Initialize(nsRefPtr<GLC
> 
>   mFBOTextureTarget = LOCAL_GL_NONE;
> 
>-  for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(textureTargets); i++) {
>+  for (PRUint32 i = 0; i < ArrayLength(textureTargets); i++) {
>     GLenum target = textureTargets[i];
>     mGLContext->fGenTextures(1, &mBackBufferTexture);
>     mGLContext->fBindTexture(target, mBackBufferTexture);
>@@ -1087,7 +1089,7 @@ LayerManagerOGL::ProgramType LayerManage
> 
> #define FOR_EACH_LAYER_PROGRAM(vname)                       \
>   for (size_t lpindex = 0;                                  \
>-       lpindex < NS_ARRAY_LENGTH(sLayerProgramTypes);       \
>+       lpindex < ArrayLength(sLayerProgramTypes);       \

Whitespace nit.

>diff --git a/gfx/thebes/gfxASurface.cpp b/gfx/thebes/gfxASurface.cpp
>--- a/gfx/thebes/gfxASurface.cpp
>+++ b/gfx/thebes/gfxASurface.cpp
>@@ -36,6 +36,8 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include "mozilla/Util.h"
>+
> #include "nsIMemoryReporter.h"
> #include "nsMemory.h"
> #include "CheckedInt.h"
>@@ -86,6 +88,7 @@
> #include "nsServiceManagerUtils.h"
> #include "nsStringGlue.h"
> 
>+using namespace mozilla;
> using mozilla::CheckedInt;
> 
> static cairo_user_data_key_t gfxasurface_pointer_key;

Vestige?

>diff --git a/layout/style/Declaration.cpp b/layout/style/Declaration.cpp
>--- a/layout/style/Declaration.cpp
>+++ b/layout/style/Declaration.cpp
>@@ -44,6 +44,8 @@
>  * stylesheet
>  */
> 
>+#include "mozilla/Util.h"
>+
> #include "mozilla/css/Declaration.h"
> #include "nsPrintfCString.h"
> 
>@@ -305,7 +307,7 @@ Declaration::GetValue(nsCSSProperty aPro
>       };
>       bool match = true;
>       for (const nsCSSProperty** subprops = subproptables,
>-               **subprops_end = subproptables + NS_ARRAY_LENGTH(subproptables);
>+               **subprops_end = subproptables + ArrayLength(subproptables);

ArrayEnd(subproptables)?

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>--- a/layout/style/nsRuleNode.cpp
>+++ b/layout/style/nsRuleNode.cpp
>@@ -49,6 +49,8 @@
>  * responsible for converting the rules' information into computed style
>  */
> 
>+#include "mozilla/Util.h"
>+
> #include "nsRuleNode.h"
> #include "nscore.h"
> #include "nsIServiceManager.h"
>@@ -3866,7 +3868,7 @@ nsRuleNode::ComputeDisplayData(void* aSt
>   // CSS Transitions
>   PRUint32 numTransitions =
>     CountTransitionProps(transitionPropInfo, transitionPropData,
>-                         NS_ARRAY_LENGTH(transitionPropData),
>+                         ArrayLength(transitionPropData),
>                          display, parentDisplay, aRuleData,
>                          canStoreInRuleTree);
> 
>@@ -4025,7 +4027,7 @@ nsRuleNode::ComputeDisplayData(void* aSt
> 
>   PRUint32 numAnimations =
>     CountTransitionProps(animationPropInfo, animationPropData,
>-                         NS_ARRAY_LENGTH(animationPropData),
>+                         ArrayLength(animationPropData),
>                          display, parentDisplay, aRuleData,
>                          canStoreInRuleTree);
> 
>@@ -4767,7 +4769,7 @@ struct BackgroundItemComputer<nsCSSValue
>   {
>     nsStyleBackground::Position &position = aComputedValue;
>     for (const BackgroundPositionAxis *axis = gBGPosAxes,
>-                        *axis_end = gBGPosAxes + NS_ARRAY_LENGTH(gBGPosAxes);
>+                        *axis_end = gBGPosAxes + ArrayLength(gBGPosAxes);

ArrayEnd(gBGPosAxes)?

>          axis != axis_end; ++axis) {
>       const nsCSSValue &specified = aSpecifiedValue->*(axis->specified);
>       if (eCSSUnit_Percent == specified.GetUnit()) {
>@@ -4829,7 +4831,7 @@ struct BackgroundItemComputer<nsCSSValue
>   {
>     nsStyleBackground::Size &size = aComputedValue;
>     for (const BackgroundSizeAxis *axis = gBGSizeAxes,
>-                        *axis_end = gBGSizeAxes + NS_ARRAY_LENGTH(gBGSizeAxes);
>+                        *axis_end = gBGSizeAxes + ArrayLength(gBGSizeAxes);

ArrayEnd(gBGSizeAxes)?

>diff --git a/mfbt/Util.h b/mfbt/Util.h
>--- a/mfbt/Util.h
>+++ b/mfbt/Util.h
>@@ -420,6 +420,29 @@ PointerRangeSize(T* begin, T* end)
>     return (size_t(end) - size_t(begin)) / sizeof(T);
> }
> 
>+/*
>+ * Compute the length of an array with constant length.  (Use of this method
>+ * with a non-array pointer will not compile.)
>+ *
>+ * Beware of the implicit trailing '\0' when using this with string constants.
>+ */
>+template<typename T, size_t N> size_t

Nit: size_t its own line please, so that the signature scans more easily.

>+ArrayLength(T (&arr)[N])
>+{
>+    return N;
>+}
>+
>+/*
>+ * Compute the address one past the last element of a constant-length array.
>+ *
>+ * Beware of the implicit trailing '\0' when using this with string constants.
>+ */
>+template<typename T, size_t N> T*

Same here with T*.

>diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp
>--- a/toolkit/crashreporter/nsExceptionHandler.cpp
>+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
>@@ -37,6 +37,8 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include "mozilla/Util.h"
>+
> #include "mozilla/dom/CrashReporterChild.h"
> #include "nsXULAppAPI.h"
> 
>@@ -114,6 +116,7 @@ CFStringRef reporterClientAppID = CFSTR(
> 
> using google_breakpad::CrashGenerationServer;
> using google_breakpad::ClientInfo;
>+using namespace mozilla;
> using mozilla::Mutex;
> using mozilla::MutexAutoLock;

You can get rid of the separate Mutex and MutexAutoLock statements.

r=me with those fixes.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 566130 [details] [diff] [review]
2 - Get rid of most JS_ARRAY_LENGTH/END uses

>diff --git a/js/src/jsemit.cpp b/js/src/jsemit.cpp
>--- a/js/src/jsemit.cpp
>+++ b/js/src/jsemit.cpp
>@@ -46,6 +46,9 @@
> #endif
> #include <new>
> #include <string.h>
>+
>+#include "mozilla/Util.h"
>+
> #include "jstypes.h"
> #include "jsstdint.h"
> #include "jsutil.h"
>@@ -84,6 +87,7 @@
> #define BYTECODE_SIZE(n)        ((n) * sizeof(jsbytecode))
> #define SRCNOTE_SIZE(n)         ((n) * sizeof(jssrcnote))
> 
>+using namespace mozilla;
> using namespace js;
> using namespace js::gc;
> 

Vestige?

>diff --git a/js/src/jsgc.h b/js/src/jsgc.h
>--- a/js/src/jsgc.h
>+++ b/js/src/jsgc.h
>@@ -45,6 +45,8 @@
>  */
> #include <setjmp.h>
> 
>+#include "mozilla/Util.h"
>+
> #include "jstypes.h"
> #include "jsprvtd.h"
> #include "jspubtd.h"
>@@ -59,6 +61,8 @@
> #include "jsgcstats.h"
> #include "jscell.h"
> 
>+using namespace mozilla;
>+

It's not kosher in gecko code to import a namespace into a common
header.  What are the js/src rules?  Actually, on second thought this
is probably included into gecko stuff, so best to steer clear.

> struct JSCompartment;
> 
> extern "C" void
>@@ -1109,7 +1113,7 @@ struct ArenaLists {
> 
>     void checkEmptyFreeLists() {
> #ifdef DEBUG
>-        for (size_t i = 0; i != JS_ARRAY_LENGTH(freeLists); ++i)
>+        for (size_t i = 0; i < ArrayLength(freeLists); ++i)

Easier to make this mozilla::ArrayLength.

>diff --git a/js/src/xpconnect/src/nsXPConnect.cpp b/js/src/xpconnect/src/nsXPConnect.cpp
>--- a/js/src/xpconnect/src/nsXPConnect.cpp
>+++ b/js/src/xpconnect/src/nsXPConnect.cpp
>@@ -42,6 +42,8 @@
> 
> /* High level class and public functions implementation. */
> 
>+#include "mozilla/Util.h"
>+
> #include "xpcprivate.h"
> #include "XPCWrapper.h"
> #include "nsBaseHashtable.h"
>@@ -69,6 +71,8 @@
> #include "dombindings.h"
> #include "nsWrapperCacheInlines.h"
> 
>+using namespace mozilla;
>+
> NS_IMPL_THREADSAFE_ISUPPORTS7(nsXPConnect,
>                               nsIXPConnect,
>                               nsISupportsWeakReference,

Vestige?

r=me with fixes above.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I think it would be possible to define a safer version of NS_ARRAY_LENGTH() at least for C++ code, but it would be ugly.  I think you could do it with a combination of a static assert that the argument isn't pointer type, plus leaving behind a constexpr to be used later.  Something like,

  #define NS_ARRAY_LENGTH(foo) \
    // static assert that foo isn't a pointer \
    // define something parameterized on sizeof(foo)/sizeof(foo[0]) named foo_length

  NS_ARRAY_LENGTH(bar)
  x = Blah<bar_length::value>(...)

May not be worth the trouble.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also, I assume a .platform and/or blog post is forthcoming to deprecate NS_ARRAY_LENGTH?

Comment 11

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da6e9073b431
https://hg.mozilla.org/integration/mozilla-inbound/rev/983533a03d0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/829074f75a55

(In reply to Chris Jones [:cjones] [:warhammer] from comment #8)
> Comment on attachment 566130 [details] [diff] [review] [diff] [details] [review]
> >diff --git a/js/src/jsgc.h b/js/src/jsgc.h
> 
> It's not kosher in gecko code to import a namespace into a common
> header.  What are the js/src rules?  Actually, on second thought this
> is probably included into gecko stuff, so best to steer clear.
> 
> >-        for (size_t i = 0; i != JS_ARRAY_LENGTH(freeLists); ++i)
> >+        for (size_t i = 0; i < ArrayLength(freeLists); ++i)
> 
> Easier to make this mozilla::ArrayLength.

Yeah, this was pure oversight -- must have missed tweaking this one header.  It's a bug that jsgc.h is probably included into gecko stuff, if it is, but regardless, it should be doing the more-qualified way.

Blog post and newsgroup mail to follow.

Comment 12

[Jeff Walden [:Waldo]]

And all backed out, compilers hate me.  Will work on/land after tryservering demonstrates happy.

Comment 13

[Jeff Walden [:Waldo]]

Relanded, with a bunch more changes for a variety of problems:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b9d9f379db
https://hg.mozilla.org/integration/mozilla-inbound/rev/88b9a330ff8c

First, it seems my compiler is more lenient than any of the tinderbox compilers in allowing mozilla::ArrayLength calls in contexts requiring a constant, like static asserts -- at least for functions that it figures out how to inline, or something.  So I missed a few places that did that.  I reverted these places (and also a few that didn't cause errors, but might have caused extra static constructors, like assigning to static const variables).

Second, my compiler apparently accepts ArrayLength calls in places like for the lengths of function-local arrays.  In C99 those lengths can be variable, but not in C++.  So compilers that didn't implement the C99 behavior, as a C++ extension, complained.  I reverted these places.

Third, my Linux system seems to have missed all the places where the array being used had a type that was unnamed, or was a local name, in non-Linux code.  I gave a bunch of structs names, and moved a bunch to global scope, to cope.

Fourth, and most perniciously: it turns out that mfbt/Utils.h includes mfbt/Types.h includes jstypes.h includes jsotypes.h.  jsotypes adds int8, int16, int32, and so on.  It also turns out that a Chromium header, base/basictypes.h, adds these same types -- only on Windows.  That header's aware of this conflict in one case, and it includes a check to error out if include order is wrong.  But it's not aware of jsotypes.h.  And in any case, "awareness" would mean telling you to include in the order I didn't use.  Across 300-odd files changed, there were about 20 instances of this where I had to change my initial ordering; I ended up Schlemiel-the-painter-ing my way through fixing all of them.  Manually checking all 300 files wouldn't have been any more practical.  The base/basictypes.h header gets included through the generated IPDL header stuff, so the number of headers that might induce a conflict is too large to tell by inspection.  Scripting up greps of includes via preprocessor output might have been feasible, but it probably wouldn't have come to much more or less effort in the end, given fixing was fast and I could do other work during intermediate compiles.

I have some ideas about how to fix this fourth problem, but for right now, people are going to have to be aware that if they want to use mozilla/Utils.h for any reason, indeed even if they want it for some reason other than mozilla::ArrayLength or mozilla::ArrayEnd, they'll have to watch out for Windows-specific compilation failures.  :-(

Comment 14

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b9b9d9f379db
https://hg.mozilla.org/mozilla-central/rev/88b9a330ff8c

Comment 15

[Marco Bonardo [:mak]]

May I ask to announce this kind of tree-wide changes in m.d.platform BEFORE landing? Since my patches queue broke and I had to manually figure out what happened going through all the recent changesets.

Comment 16

[Marco Bonardo [:mak]]

I also wonder if you want to replace the 2 remaining uses of PR_ARRAY_SIZE in xpcom
http://mxr.mozilla.org/mozilla-central/search?string=PR_ARRAY_SIZE&find=xpcom&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 17

[Jeff Walden [:Waldo]]

Sure, you can ask.  A bit late this time to do anything, tho.  :-\

That said, why particularly should this have been announced beforehand?  I didn't remove the old macros, so new code that uses them shouldn't break.  People with inflight patches that touched code this bug touched would be affected, sure.  But how is that any different from any other bug and patch, except in how spread out the changes are, and how many different places were changed?  Seems like par for the course for anyone working on any patches, really, that you have to adapt to changes simultaneously made by others.

Thanks for pointing out PR_ARRAY_SIZE, that can/should indeed be fixed.

Blog post still coming shortly.

Comment 18

[Marco Bonardo [:mak]]

(In reply to Jeff Walden (remove +bmo to email) from comment #17)
> sure.  But how is that any different from any other bug and patch,
> except in how spread out the changes are, and how many different places were
> changed?

Just that, changes that spread across all the codebase (multiple modules) may be surprising when you have large patches in queue. I fixed the issue locally pretty fast, but still I was not aware where to look for the change and spent more time figuring that out. When large changes affect a single module it's more likely whoever is working on it may be aware of the incoming change.

Comment 19

[Jeff Walden [:Waldo]]

Blog posts:

https://whereswalden.com/2011/10/20/computing-the-length-or-end-of-a-compile-time-constant-length-array-jsns_array_endlength-are-out-mozillaarraylengthend-are-in/
https://whereswalden.com/2011/10/20/implementing-mozillaarraylength-and-mozillaarrayend-and-some-followup-work/

And I sent mail to .platform about this as well, mentioning those posts too:

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/e060237273029f67#

And if you're wondering about the bug 696242 dependency I just added, that's for some followup work to this that would eliminate a lot of the ArrayEnd calls we have now, by pushing the templatize-on-array-length trick a bit further down so that more callers can just pass an array and don't have to pass its length at the same time.

Comment 20

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/107c941e27dc

to fix a few stragglers, possibly added after I did my initial s&r, also picking up PR_ARRAY_SIZE uses at the same time.