883574 - Miscellaneous const qualifiers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

Numerous variables and pointers in the JS tree could have const applied to them. This sometimes helps the compiler allocate things in read-only memory, and sometimes it can help catch programmer errors.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: a proposed fix

Comment 2

[Tom S [:evilpie]]

Comment on attachment 763162 [details] [diff] [review]
a proposed fix

Review of attachment 763162 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. I am however not sure if everybody is a fan of change. I personally find at least the cases of "const X *const Y;" harder to read. The consifification of functions is awesome and I think nobody has a problem with that. I think Jeff is the right person to decide this.

::: js/jsd/jsd_text.cpp
@@ +171,5 @@
>              return tmp;
>      }
>  }
>  
> +static const char file_url_prefix[]    = "file:";

Remove the space please.

::: js/src/jsdate.cpp
@@ +2459,5 @@
>  }
>  
>  /* constants for toString, toUTCString */
> +static const char js_NaN_date_str[] = "Invalid Date";
> +static const char* const days[] =

prexisting, move to right.

::: js/src/jsgc.cpp
@@ +175,5 @@
>  static const AllocKind FinalizePhaseIonCode[] = {
>      FINALIZE_IONCODE
>  };
>  
> +static const AllocKind* const FinalizePhases[] = {

here, too

@@ +218,5 @@
>      FINALIZE_BASE_SHAPE,
>      FINALIZE_TYPE_OBJECT
>  };
>  
> +static const AllocKind* const BackgroundPhases[] = {

also here

::: js/src/jsopcode.cpp
@@ +2083,5 @@
>  }
>  
>  static void
>  AppendArrayJSONProperties(JSContext *cx, StringBuffer &buf,
> +                          double *values, const char *const *names, unsigned count,

I really find this painful to read. I blame C++.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 763162 [details] [diff] [review]
a proposed fix

Review of attachment 763162 [details] [diff] [review]:
-----------------------------------------------------------------

Adding const to void* types seems somewhat unlikely to make any difference to me, but eh.  :-)

::: js/src/TraceLogging.h
@@ +60,5 @@
>      unsigned int numEntries;
>      int fileno;
>      FILE *out;
>  
> +    const static char *const type_name[];

I'd slightly prefer seeing |static const| as the canonical order, not |const static|.  The former gets the storage class out of the way, then proceeds to the type.  The latter starts with part of the type -- const -- then goes to storage -- static -- then back to type.  That's literally mixed-up.  :-)  It looks like the overall js/src numbers back this up, too -- 44 hits for "const static", 1000+ for "static const".

So let's switch this to |static const| while you're touching this.  Same for any other cases you might be touching elsewhere in this patch.

::: js/src/assembler/assembler/ARMAssembler.h
@@ +1202,5 @@
>          static char const * nameGpReg(int reg)
>          {
>              ASSERT(reg <= 16);
>              ASSERT(reg >= 0);
> +            static char const *const names[] = {

I think this reads slightly better with the * not smack up against the const, generally.  (And with the * next to the type, but I haven't won that battle yet.  :-) )  So:

  static const char * const names[] = {

Looks like a search for "*const" should find all such instances pretty easily.

::: js/src/jsopcode.cpp
@@ +72,5 @@
>  #include "jsopcode.tbl"
>  #undef OPDEF
>  };
>  
> +unsigned const js_NumCodeSpecs = JS_ARRAY_LENGTH(js_CodeSpec);

const unsigned, please.

@@ +2083,5 @@
>  }
>  
>  static void
>  AppendArrayJSONProperties(JSContext *cx, StringBuffer &buf,
> +                          double *values, const char *const *names, unsigned count,

Agreed somewhat about the pain.  For non-string types, I tend to think typedefs get helpful once you have much */const-nesting going on.  For strings, a typedef seems kinda overkillish.  Bleh, not sure there's a good solution.  Oh well -- guess we suck it up in the interests of efficiency.

Comment 4

[Dan Gohman [:sunfish]]

Thanks for the reviews! I know a const patch isn't the most fun to review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4967e7c6da7

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b4967e7c6da7