[Webkit-unassigned] [Bug 103707] AssemblyHelpers::decodedCodeMapFor crash

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=103707





--- Comment #21 from Filip Pizlo <fpizlo at apple.com>  2013-12-02 10:54:51 PST ---
(From update of attachment 181285)
View in context: https://bugs.webkit.org/attachment.cgi?id=181285&action=review

>> Source/JavaScriptCore/jit/JIT.cpp:754
>> +/*!! end change by linzj !!*/
> 
> Eliminate the /*!! … !!*/ comments.
> Not sure how many of the other comments are needed.  If you still think some/all of these comment need to be present, make these complete sentences beginning with a capitalized word and with a space after the '.'

Yeah, I mean, this change needs a lot of work.  You shouldn't be adding comments that say that it's your change; this is a bad style.  It doesn't scale.  Imagine if everyone who changed the code added such a comment; pretty soon you would have a giant mess.  The comment isn't accurate.  The change itself has some sloppiness.  First, it's bad because you're introducing an "if (true) ..." into the code.  Please don't do that.  If you have something that is unconditional and must always be executed, then you should remove the "if".  The change also assumes that the DFG can jump to any baseline code block; this is not true - it can only jump to those code blocks that can be inlined.  There is already machinery in this file for detecting whether a code block is inlineable.

I wouldn't be opposed to just making the CompactJITCodeMap creation unconditional on the grounds that we don't care about optimizing !ENABLE(LLINT) builds.  This change accomplishes this functionally but stylistically it needs a lot of work before it can be accepted into the repository.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.