[webkit-reviews] review denied: [Bug 43619] Bitmap.h has no default constructor : [Attachment 63718] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 6 10:45:37 PDT 2010


Geoffrey Garen <ggaren at apple.com> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 43619: Bitmap.h has no default constructor
https://bugs.webkit.org/show_bug.cgi?id=43619

Attachment 63718: patch
https://bugs.webkit.org/attachment.cgi?id=63718&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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!


More information about the webkit-reviews mailing list