Bug 132854 - Remove check-webkit-style warnings in JavascriptCore/dfg
Summary: Remove check-webkit-style warnings in JavascriptCore/dfg
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-12 23:57 PDT by Tanay
Modified: 2014-05-19 02:01 PDT (History)
1 user (show)

See Also:


Attachments
Patch (8.51 KB, patch)
2014-05-14 03:11 PDT, Tanay
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2014-05-15 02:46 PDT, Tanay
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tanay 2014-05-12 23:57:43 PDT
Remove warnings of type :
*The parameter name x adds no information, so it should be removed.
Comment 1 Tanay 2014-05-14 03:11:03 PDT
Created attachment 231440 [details]
Patch
Comment 2 Darin Adler 2014-05-14 09:27:02 PDT
Comment on attachment 231440 [details]
Patch

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

Most of the changes look like small improvements, but some of them make things a little worse and should not be done.

> Source/JavaScriptCore/dfg/DFGCapabilities.h:50
> -inline CapabilityLevel capabilityLevel(OpcodeID opcodeID, CodeBlock* codeBlock, Instruction* pc);
> +inline CapabilityLevel capabilityLevel(OpcodeID, CodeBlock*, Instruction*);

I am not sure that removing the name “pc” makes the code more clear. I would probably leave it in.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1974
> -    void addBranch(const MacroAssembler::JumpList& jump, BasicBlock* destination);
> +    void addBranch(const MacroAssembler::JumpList&, BasicBlock*);

Removing the name “destination” makes this less clear.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2243
> +    void speculateString(Edge, GPRReg);
> +    void speculateStringIdentAndLoadStorage(Edge, GPRReg, GPRReg);
> +    void speculateStringIdent(Edge, GPRReg);

Removing the names “cell”, “string”, and “storage” here makes this less clear. Those names aren’t redundant.
Comment 3 Tanay 2014-05-14 22:37:31 PDT
(In reply to comment #2)
> (From update of attachment 231440 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231440&action=review
> 
> Most of the changes look like small improvements, but some of them make things a little worse and should not be done.
> 
> > Source/JavaScriptCore/dfg/DFGCapabilities.h:50
> > -inline CapabilityLevel capabilityLevel(OpcodeID opcodeID, CodeBlock* codeBlock, Instruction* pc);
> > +inline CapabilityLevel capabilityLevel(OpcodeID, CodeBlock*, Instruction*);
> 
> I am not sure that removing the name “pc” makes the code more clear. I would probably leave it in.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1974
> > -    void addBranch(const MacroAssembler::JumpList& jump, BasicBlock* destination);
> > +    void addBranch(const MacroAssembler::JumpList&, BasicBlock*);
> 
> Removing the name “destination” makes this less clear.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2243
> > +    void speculateString(Edge, GPRReg);
> > +    void speculateStringIdentAndLoadStorage(Edge, GPRReg, GPRReg);
> > +    void speculateStringIdent(Edge, GPRReg);
> 
> Removing the names “cell”, “string”, and “storage” here makes this less clear. Those names aren’t redundant.

Ok, I will omit the changes you mentioned above and submit a patch.
Comment 4 Tanay 2014-05-15 02:46:35 PDT
Created attachment 231497 [details]
Patch
Comment 5 WebKit Commit Bot 2014-05-19 02:01:30 PDT
Comment on attachment 231497 [details]
Patch

Clearing flags on attachment: 231497

Committed r169040: <http://trac.webkit.org/changeset/169040>
Comment 6 WebKit Commit Bot 2014-05-19 02:01:32 PDT
All reviewed patches have been landed.  Closing bug.