Bug 11822 - DeprecatedStringData allocation size is 52 bytes due to struct packing
Summary: DeprecatedStringData allocation size is 52 bytes due to struct packing
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 420+
Hardware: PC Windows XP
: P3 Minor
Assignee: Darin Fisher (:fishd, Google)
Depends on:
Reported: 2006-12-13 15:38 PST by Darin Fisher (:fishd, Google)
Modified: 2007-03-17 12:18 PDT (History)
0 users

See Also:

patch (1.42 KB, patch)
2006-12-15 18:43 PST, Darin Fisher (:fishd, Google)
ggaren: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2006-12-13 15:38:27 PST
DeprecatedStringData allocation size is 52 bytes due to struct packing

The comment in DeprecatedString.h says:

  // Keep this struct to <= 46 bytes, that's what the system will allocate.
  // Will be rounded up to a multiple of 4, so we're stuck at 44.

I have no idea how significant that comment is or whether it matters that DeprecatedStringData > 46 bytes on Windows.  Perhaps it would be good to wrap the declaration of that class with #pragma pack(push, 1) ... #pragma pack(pop) just in case.
Comment 1 Darin Fisher (:fishd, Google) 2006-12-15 18:43:14 PST
Created attachment 11874 [details]

I haven't determined if this actually is necessary, but here's a patch.
Comment 2 Geoffrey Garen 2006-12-23 16:24:36 PST
Comment on attachment 11874 [details]

I don't think we want to make this change without knowing what it accomplishes or being able to test it.

Building mysterious code on top of mysterious comments seems like a recipe for either (a) disaster or (b) a sequel to the Da Vinci Code.
Comment 3 Maciej Stachowiak 2006-12-23 23:07:38 PST
I think this comment was specific to the size classes of the Mac OS X system malloc. But WebKit will no longer use that, even on Mac OS X, so I think the best thing would be to remove the comment.
Comment 4 Darin Fisher (:fishd, Google) 2007-01-02 10:04:19 PST
My hope was that one of you guys would know what this comment was all about and be able to decide whether you want the same for windows or if the comment is just bogus.  Sounds like the latter, so I'll rev the patch accordingly.