Closed
Bug 569753
Opened 14 years ago
Closed 14 years ago
nanojit: clean up ExprFilter::ins2()
Categories
(Core Graveyard :: Nanojit, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)
Attachments
(1 file)
12.02 KB,
patch
|
edwsmith
:
review+
|
Details | Diff | Splinter Review |
ExprFilter:ins2() is 230 lines long, and really hard to read. It should be split into multiple functions.
Assignee | ||
Comment 1•14 years ago
|
||
I tried splitting it into multiple functions but it ended up being no easier to read. So I've done some cleanups within the single big function: - Tweaked spacing, esp. put numerous case statements on the same line as the case label, which makes them easier to read and lets you see more code at once. - Fixed up some inconsistent and unnecessary uses of int32_t and uint32_t casts. - Added some comments. One question: we have this "fold" case for constant +/-/* expressions where we carefully avoid overflow. Is this worth bothering with? Will the compile-time overflow not be equivalent to the run-time overflow? Maybe it's worth keeping just to be prudent, but I want to understand if there are cases where it's truly necessary to avoid changing the program's meaning.
Attachment #450291 -
Flags: review?(edwsmith)
Assignee | ||
Updated•14 years ago
|
Summary: nanojit: split up ExprFilter::ins2() → nanojit: clean up ExprFilter::ins2()
Comment 2•14 years ago
|
||
Comment on attachment 450291 [details] [diff] [review] patch I think the overflow avoidance was a holdover from the LIR_ov days. if the expression was constant then the add could be removed, leaving the LIR_ov hanging around with no input. Now, with our new LIR_*ov* variants, seems like we can let the plain operators overflow as long as the C compilers do what our jit would do. safe assumption i think, especially since we're not cross compiling. I dont think the C compilers will be able to exploit C's loopholes that overflows are undefined, because it can't see the values as constants. R+ for style, I'm assuming you're testing LIR diffs so I havent reviewed carefully for typo-style injections.
Attachment #450291 -
Flags: review?(edwsmith) → review+
Comment 3•14 years ago
|
||
style q: are we officially preferring if (condition) { over if (condition) { in nanojit? (IMHO the latter makes for easier to read code on modern giganto-screens, and is more prevalent in TR code, but I'm not religious about it...)
Assignee | ||
Comment 4•14 years ago
|
||
NJ: http://hg.mozilla.org/projects/nanojit-central/rev/27e933f40d36 TM: http://hg.mozilla.org/tracemonkey/rev/895516ddc0be
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey
Assignee | ||
Comment 5•14 years ago
|
||
(In reply to comment #2) > (From update of attachment 450291 [details] [diff] [review]) > I think the overflow avoidance was a holdover from the LIR_ov days. if the > expression was constant then the add could be removed, leaving the LIR_ov > hanging around with no input. > > Now, with our new LIR_*ov* variants, seems like we can let the plain operators > overflow as long as the C compilers do what our jit would do. safe assumption > i think, especially since we're not cross compiling. I dont think the C > compilers will be able to exploit C's loopholes that overflows are undefined, > because it can't see the values as constants. I left the overflow checking in (paranoia) and added a comment. (In reply to comment #3) > style q: are we officially preferring > > if (condition) { > > over > > if (condition) > { > > in nanojit? Let me consult the NJ style guide... oh, there isn't one :) I prefer the former and so tend to use it. Unless we initiate a style guide I think there will always be inconsistencies.
Comment 6•14 years ago
|
||
TR: http://hg.mozilla.org/tamarin-redux/rev/3d4bf0e0a2cd
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Comment 7•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/895516ddc0be
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•