Bug 102572

Summary: Don't blind all the things.
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: JavaScriptCoreAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch barraclough: review+

Description Oliver Hunt 2012-11-16 16:56:32 PST 
Don't blind all the things.
Comment 1 Oliver Hunt 2012-11-26 12:29:41 PST 
Created attachment 176046 [details]
Patch
Comment 2 Darin Adler 2012-11-26 12:37:06 PST 
Comment on attachment 176046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176046&action=review

> Source/JavaScriptCore/ChangeLog:12
> +        (MacroAssembler):

Please delete bogus lines like this from change log.

> Source/JavaScriptCore/assembler/MacroAssembler.h:843
> +    bool shouldConsiderBlinding()

Should be a static member function?

> Source/JavaScriptCore/assembler/MacroAssembler.h:847
>      bool shouldBlindDouble(double value)

Should be a static member function?

> Source/JavaScriptCore/assembler/MacroAssembler.h:895
> +            if (~value <= 0xff)
> +                return false;

No explanation of this change in the change log.

> Source/JavaScriptCore/assembler/MacroAssembler.h:954
> +            if (~value <= 0xff)
> +                return false;

No explanation of this change in the change log.

> Source/JavaScriptCore/assembler/MacroAssembler.h:1089
> +            if (~value <= 0xff)
> +                return false;

No explanation of this change in the change log.
Comment 3 Gavin Barraclough 2012-11-26 13:13:37 PST 
Comment on attachment 176046 [details]
Patch

r=me, please add changelog comment re blinding small negative value.
Comment 4 Oliver Hunt 2012-11-26 13:21:17 PST 
Committed r135757: <http://trac.webkit.org/changeset/135757>
Comment 5 Darin Adler 2012-11-27 10:03:02 PST 
Comment on attachment 176046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176046&action=review

>> Source/JavaScriptCore/assembler/MacroAssembler.h:843
>> +    bool shouldConsiderBlinding()
> 
> Should be a static member function?

Why is this a non-static member function instead of a static one?

>> Source/JavaScriptCore/assembler/MacroAssembler.h:847
>>      bool shouldBlindDouble(double value)
> 
> Should be a static member function?

Why is this a non-static member function instead of a static one?
Comment 6 Oliver Hunt 2012-11-27 10:41:33 PST 
(In reply to comment #5)
> (From update of attachment 176046 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176046&action=review
> 
> >> Source/JavaScriptCore/assembler/MacroAssembler.h:843
> >> +    bool shouldConsiderBlinding()
> > 
> > Should be a static member function?
> 
> Why is this a non-static member function instead of a static one?
> 
> >> Source/JavaScriptCore/assembler/MacroAssembler.h:847
> >>      bool shouldBlindDouble(double value)
> > 
> > Should be a static member function?
> 
> Why is this a non-static member function instead of a static one?

Because they use member functions