WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
125215
Avoid to add zero offset in BaseIndex.
https://bugs.webkit.org/show_bug.cgi?id=125215
Summary
Avoid to add zero offset in BaseIndex.
László Langó
Reported
2013-12-04 02:50:05 PST
Avoid to add zero offset in BaseIndex class in offlineasm/cloop.rb.
Attachments
Patch
(1.51 KB, patch)
2013-12-04 02:53 PST
,
László Langó
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
László Langó
Comment 1
2013-12-04 02:53:00 PST
Created
attachment 218394
[details]
Patch
WebKit Commit Bot
Comment 2
2013-12-04 06:39:23 PST
Comment on
attachment 218394
[details]
Patch Clearing flags on attachment: 218394 Committed
r160091
: <
http://trac.webkit.org/changeset/160091
>
WebKit Commit Bot
Comment 3
2013-12-04 06:39:25 PST
All reviewed patches have been landed. Closing bug.
Filip Pizlo
Comment 4
2013-12-04 09:14:08 PST
As a general rule, I would avoid making such changes. This change has no upside. The C compiler would have folded the zero, and it would have done it as part of one of many already-mandatory folding passes - even at -O0. So, you're just duplicating that functionality in offlineasm, which just means more code to maintain and more places where someone might make a mistake.
László Langó
Comment 5
2013-12-05 01:08:57 PST
(In reply to
comment #4
)
> As a general rule, I would avoid making such changes. This change has no upside. The C compiler would have folded the zero, and it would have done it as part of one of many already-mandatory folding passes - even at -O0. So, you're just duplicating that functionality in offlineasm, which just means more code to maintain and more places where someone might make a mistake.
Thanks for the reply. Yes, you are partly right. The GCC with -O0 really optimize this out, but the clang isn't. It won't do it with -O0. The other reason why I made this is that there's already a similar change in class Address in this file:
http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/offlineasm/cloop.rb#L195
So we should keep or remove from both places. I'm still think this is a good change.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug