nanojit: clean up ExprFilter::ins2()

RESOLVED FIXED

Status

RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
ExprFilter:ins2() is 230 lines long, and really hard to read.  It should be split into multiple functions.

Updated

9 years ago
Depends on: 560106
(Assignee)

Comment 1

9 years ago
Created attachment 450291 [details] [diff] [review]
patch

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

9 years ago
Summary: nanojit: split up ExprFilter::ins2() → nanojit: clean up ExprFilter::ins2()

Comment 2

9 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

9 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 5

9 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

9 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

8 years ago
http://hg.mozilla.org/mozilla-central/rev/895516ddc0be
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.