Closed
Bug 1228340
Opened 9 years ago
Closed 9 years ago
Get rid of the js_ prefix under jit/
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: bbouvier, Assigned: bbouvier)
Details
Attachments
(3 files)
8.19 KB,
patch
|
h4writer
:
review+
Waldo
:
feedback+
|
Details | Diff | Splinter Review |
47.93 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
69.69 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
The following patches get rid of the js_ prefix for most remaining things present under jit/. All the allocators operators remain unaffected, as I don't have a good idea / plan for a name (e.g. instead of js_malloc, maybe malloc_? or _malloc?).
Assignee | ||
Comment 1•9 years ago
|
||
js_IonOptimizationLevel => IonOptimizationLevel
Assignee | ||
Comment 2•9 years ago
|
||
Rename the js_JitOptions struct to DefaultJitOptions, and get rid of the js_ prefix for the JitOptions singleton instance.
Attachment #8692540 -
Flags: review?(hv1989)
Assignee | ||
Comment 3•9 years ago
|
||
Get rid of the js_ prefix for Code{Spec,Name}.
Attachment #8692541 -
Flags: review?(jorendorff)
Comment 4•9 years ago
|
||
Comment on attachment 8692538 [details] [diff] [review] 1.ionoptimizationslevel.patch Review of attachment 8692538 [details] [diff] [review]: ----------------------------------------------------------------- Patch seems fine. I'm only not sure if 'singletons' should start with capital. In the old code, I just copied the JitOptions case, which was js_JitOptions at the time. Now removing the js_ makes this a capital. Should we lowercase this? @Waldo: do you have an idea about this?
Attachment #8692538 -
Flags: review?(hv1989)
Attachment #8692538 -
Flags: review+
Attachment #8692538 -
Flags: feedback?(jwalden+bmo)
Comment 5•9 years ago
|
||
Comment on attachment 8692540 [details] [diff] [review] 2.jitoptions.patch Review of attachment 8692540 [details] [diff] [review]: ----------------------------------------------------------------- Same comment as with IonOptimizationLevel
Attachment #8692540 -
Flags: review?(hv1989) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8692538 [details] [diff] [review] 1.ionoptimizationslevel.patch Review of attachment 8692538 [details] [diff] [review]: ----------------------------------------------------------------- I think our convention is to use a leading capital letter for globally-scoped variables, so JitOptions would be right. That said, I'm not a big fan of it, and I'd rather we had class-scoped variables with lowercase names, wherever it makes sense to do so. But that's out of scope for a pure renaming patch/bug, IMO. ::: js/src/jit/IonOptimizationLevels.cpp @@ +15,5 @@ > > namespace js { > namespace jit { > > +OptimizationInfos IonOptimizations; "Info" usually isn't plural, it's a collective noun, so this likely should be OptimizationInfo. But a separate thing, not this bug.
Attachment #8692538 -
Flags: feedback?(jwalden+bmo) → feedback+
Comment 7•9 years ago
|
||
Comment on attachment 8692541 [details] [diff] [review] 3.codespecname.patch Review of attachment 8692541 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Interpreter.cpp @@ +1957,5 @@ > JS_END_MACRO > > #define TRY_BRANCH_AFTER_COND(cond,spdec) \ > JS_BEGIN_MACRO \ > + MOZ_ASSERT(CodeSpec[*REGS.pc].length == 1); \ Micro-nit: Please re-align the \ at the end of this line.
Attachment #8692541 -
Flags: review?(jorendorff) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd50837b1f0c https://hg.mozilla.org/integration/mozilla-inbound/rev/91bfd1093c2c https://hg.mozilla.org/integration/mozilla-inbound/rev/526a9c02295b
Assignee | ||
Comment 9•9 years ago
|
||
Thank you all for the quick reviews! Bug 1229338 addresses the renaming of OptimizationInfos.
Comment 10•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dd50837b1f0c https://hg.mozilla.org/mozilla-central/rev/91bfd1093c2c https://hg.mozilla.org/mozilla-central/rev/526a9c02295b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•