Bug 108247 – Remove redundant AST dump method from cloop.rb, since they are already defined in ast.rb

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

The definitions in ast.rb are the correct ones since they pretty-print the code that you would have written.  The ones in cloop.rb appear to do something different entirely, and often lead to either ambiguities (AbsoluteAddress will print the same as Immediate), strange assertions (RegisterID will assert the name of the register, and it's not clear it supports the full list of registers), and general confusion (RegisterID and BaseIndex use register names that are not valid offlineasm syntax).

[Filip Pizlo]

Created attachment 185336 [details]
the patch

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/141178

[Mark Lam]

Comment on attachment 185336 [details]
the patch

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

> Source/JavaScriptCore/offlineasm/cloop.rb:-96
> -    end

I think you will break the cloop code with this change.  See LowLevelInterpreter.cpp.  These register names are used there.  If you change these, I think you'll have to change it there as well to allow LLINT_C_LOOP to build.

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 185336 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185336&action=review
> 
> > Source/JavaScriptCore/offlineasm/cloop.rb:-96
> > -    end
> 
> I think you will break the cloop code with this change.  See LowLevelInterpreter.cpp.  These register names are used there.  If you change these, I think you'll have to change it there as well to allow LLINT_C_LOOP to build.

Ah.  In that case the correct thing to do is to not have cloop.rb use a method called 'dump'.  That's the bug: https://bugs.webkit.org/show_bug.cgi?id=108251