Various RegExpPrivate cleanups

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Unassigned)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 572927 [details] [diff] [review]
Patch

Right now the OS X opt warn-as-errors shell build is red because the compiler can't tell that a RegExpPrivateCache::AddPtr will never be used uninitialized.  I fixed that with some slight refactoring which also moves not-cached cases out-of-line.  I also noticed some type changes mean that we have pointless ensureLinear(cx) calls a few places, so I removed those.  (I have an idea for avoiding such mistakes in the future, but I'll leave that to a separate bug.)
Attachment #572927 - Flags: review?(cdleary)
Comment on attachment 572927 [details] [diff] [review]
Patch

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

Thanks, looks good.

::: js/src/vm/RegExpObject.cpp
@@ +469,4 @@
>      return true;
>  }
>  
> +/* static */ RegExpPrivate *

Nit: I don't think we usually make these comments, do we? Without precedent or a style guide note, I think I'd rather we leave it out.

@@ +469,5 @@
>      return true;
>  }
>  
> +/* static */ RegExpPrivate *
> +RegExpPrivate::createUncached(JSContext *cx, JSLinearString *source, RegExpFlag flags, TokenStream *ts)

Nit: I think we're s/ts/tokenStream these days while we're in here, right?
Attachment #572927 - Flags: review?(cdleary) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9cc509bfb07

There seems to be a reasonable amount of precedent for /* static */, so I left that as-is.  I did change s/ts/tokenStream/, although for utterly trivial cases like that an initialism doesn't really bother me much.
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/d9cc509bfb07
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.