Bug 43619 - Bitmap.h has no default constructor
Summary: Bitmap.h has no default constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 41177
  Show dependency treegraph
 
Reported: 2010-08-06 05:57 PDT by Zoltan Herczeg
Modified: 2010-12-02 12:47 PST (History)
4 users (show)

See Also:


Attachments
patch (1.95 KB, patch)
2010-08-06 07:11 PDT, Zoltan Herczeg
ggaren: review-
Details | Formatted Diff | Diff
patch v2 (1.75 KB, patch)
2010-08-06 12:04 PDT, Zoltan Herczeg
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Herczeg 2010-08-06 05:57:15 PDT
The bitmap array is not cleared, and contains a memory garbage after init.
Comment 1 Zoltan Herczeg 2010-08-06 07:11:43 PDT
Created attachment 63718 [details]
patch
Comment 2 Geoffrey Garen 2010-08-06 10:45:37 PDT
Comment on attachment 63718 [details]
patch

Nice catch.

JavaScriptCore/wtf/Bitmap.h:35
 +      Bitmap(bool initializationNeeded = true);
WebKit is moving toward a policy of considering boolean arguments to functions to be a bad design pattern. I think we may eventually discourage or forbid them in our coding style guidelines. The problem with this kind of argument is that it's very hard to tell, at the callsite, exactly what "true" or "false" might mean.

I think you should just remove the boolean argument here.

If we do want to take advantage of an optimized "no initialization" bitmap in the future, let's add an extra constructor akin to the AdoptCFTag constructor for RetainPtr, or the VPtrStealingHackType constructor for JSString.

Otherwise, this patch is great!
Comment 3 Zoltan Herczeg 2010-08-06 12:04:47 PDT
Created attachment 63748 [details]
patch v2
Comment 4 Eric Seidel 2010-08-06 13:31:05 PDT
Comment on attachment 63748 [details]
patch v2

LGTM.
Comment 5 Zoltan Herczeg 2010-08-07 10:37:35 PDT
Landed in http://trac.webkit.org/changeset/64912
Closing bug.
Comment 6 Yael 2010-12-02 12:21:45 PST
This is the fix I was about to suggest for https://qtrequirements.europe.nokia.com/browse/BR-4872, just to find that it was already fixed. :-(
Comment 7 Ademar Reis 2010-12-02 12:47:18 PST
Revision r64912 cherry-picked into qtwebkit-2.1 with commit 67daffa <http://gitorious.org/webkit/qtwebkit/commit/67daffa>