I looked through all of the .h files and some of the .cc files. All of the
changes appear good (although it would be very easy to miss an error). Some
of
the changes look like they will reduce the size of structures.
Some of the changes are avoidable (I commented on some but not all of
these). If
you have a set of bit fields where all of the items are bool then should
will be
fine on all compilers and arguably should be left alone to minimize
churn/risk.
Similarly if you have a bit field where all of the items are int or
unsigned int
or other 32-bit types then all is well.
It is only when mixing sizes (int and bool for instance) or using enums in
bit
fields that things go awry.
Even some of the enum cases could be solved more simply by just not using a
bit
field. There are some cases where bit fields are being used where simple
typed
enums with no bit fields would also work.
I would recommend only using the BitFields class where it offers a benefit
over
normal bit fields and over just avoiding bit fields entirely.
https://codereview.chromium.org/700963002/diff/1/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/700963002/diff/1/src/ast.h#newcode427
src/ast.h:427: uint32_t bit_field_;
Why use a bit field at all? byte/bool/bool take up three bytes, pads to
four bytes, which should be perfectly space efficient, more run-time
efficient.
https://codereview.chromium.org/700963002/diff/1/src/heap-snapshot-generator.h
File src/heap-snapshot-generator.h (left):
https://codereview.chromium.org/700963002/diff/1/src/heap-snapshot-generator.h#oldcode56
src/heap-snapshot-generator.h:56: int from_index_ : 29;
FWIW, I believe that this usage of bit fields is portable. Certainly
VC++ handles it fine.
https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (left):
https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h#oldcode1530
src/hydrogen-instructions.h:1530: bool map_is_stable_ : 1;
Good fix. VC++ would handle this type of bit field poorly -- the
map_is_stable_ member would be in a separate backing store so that they
would use 64 bits instead of 32.
https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h#oldcode1900
src/hydrogen-instructions.h:1900: bool done_with_replay_ : 1;
Also a good fix.
https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h#oldcode6960
src/hydrogen-instructions.h:6960: StoreFieldOrKeyedMode store_mode_ : 1;
Good fix. Could also use separate fields and a typed enum.
https://codereview.chromium.org/700963002/diff/1/src/hydrogen-instructions.h#oldcode7175
src/hydrogen-instructions.h:7175: StoreFieldOrKeyedMode store_mode_: 1;
Could also fix with two non-bit field bools and a typed enum.
https://codereview.chromium.org/700963002/diff/1/src/profile-generator.h
File src/profile-generator.h (left):
https://codereview.chromium.org/700963002/diff/1/src/profile-generator.h#oldcode92
src/profile-generator.h:92: Builtins::Name builtin_id_ : 8;
Could presumably use typed enums (unsigned char) and no bit fields.
https://codereview.chromium.org/700963002/diff/1/src/unicode.h
File src/unicode.h (left):
https://codereview.chromium.org/700963002/diff/1/src/unicode.h#oldcode40
src/unicode.h:40: bool value_ : 1;
Good fix. VC++ cannot handle this unless value_ is also stored in a
32-bit type (unsigned int).
https://codereview.chromium.org/700963002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
