| Summary: | [v8] in Debug builds create an additional handle scope when doing debug build checks | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | anton muhin <antonm> | ||||||
| Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aroben, commit-queue, eric, evan, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
anton muhin
2009-12-09 11:35:59 PST
Created attachment 44551 [details]
first take
style-queue ran check-webkit-style on attachment 44551 [details] without any errors.
Comment on attachment 44551 [details]
first take
Seems like the ASSERTS can be outside of the #ifdef. How much does it cost to create the handle scope unconditionally? I'd rather not have NDEBUG everywhere.... Can we create the handle scope at a lower level where we're already splitting on NDEBUG?
(In reply to comment #3) > (From update of attachment 44551 [details]) > Seems like the ASSERTS can be outside of the #ifdef. Technically yes. However, if I remember it right, one can compile WebKit w/ asserts, but in Release mode, so we'll be in the trouble anyway. Having said that, I'd easily adjust. > How much does it cost to create the handle scope unconditionally? I'd rather not have NDEBUG everywhere.... That's a hot path, and handle scope is only needed due to ASSERTS, so I'd rather not spend a CPU cycle on it. If you don't mind. > Can we create the handle scope at a lower level where we're > already splitting on NDEBUG? Yes, for maybeDOMWrapper, but there is this type check, alas. Overall, I wholeheartedly agree those NDEBUG are ugly. So just let me know how you'd like that to be arranged. Comment on attachment 44551 [details]
first take
Ah! I misunderstood. It's the ASSERTS themselves that require the handle scope. Yeah, I don't see a more aesthetic way to do this. Thanks for explaining it to me.
(In reply to comment #5) > (From update of attachment 44551 [details]) > Ah! I misunderstood. It's the ASSERTS themselves that require the handle > scope. Yeah, I don't see a more aesthetic way to do this. Thanks for > explaining it to me. Thanks a lot for review, Adam, cq+'ing. Comment on attachment 44551 [details] first take Rejecting patch 44551 from commit-queue. Found no modified ChangeLogs, cannot create a commit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html Comment on attachment 44551 [details]
first take
Lets see if the failure reproduces. If it does, then I'll file a bug and look at the machine itself.
Comment on attachment 44551 [details] first take Rejecting patch 44551 from commit-queue. Found no modified ChangeLogs, cannot create a commit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html I'm surprised the failure didn't occur earlier. It looks like svn-apply fails with this patch:
bugzilla-tool apply-attachment 44551 [details]
I wonder if however you generated this git diff is different than how our scripts expect?
(In reply to comment #10) > I'm surprised the failure didn't occur earlier. It looks like svn-apply fails > with this patch: > > bugzilla-tool apply-attachment 44551 [details] > > I wonder if however you generated this git diff is different than how our > scripts expect? git diff --no-prefix Uploading a new patch (generated with plain git diff). Just in case: zsh/3 127 --% git --version git version 1.5.4.3 Created attachment 44683 [details]
plain git diff
style-queue ran check-webkit-style on attachment 44683 [details] without any errors.
Comment on attachment 44683 [details] plain git diff Clearing flags on attachment: 44683 Committed r51995: <http://trac.webkit.org/changeset/51995> All reviewed patches have been landed. Closing bug. So yes, git diff --no-prefix was to blame. I've filed bug 32438 about the svn-apply bug. Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the "right thing" regarding git patches. See "bugzilla-tool help" for more information. You might find one of those commands useful to you when posting patches in the future. (In reply to comment #18) > Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the > "right thing" regarding git patches. See "bugzilla-tool help" for more > information. You might find one of those commands useful to you when posting > patches in the future. Thank you very much for pointing it out---never heard of it. |