Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2231953003:
Added ASSERT_SIZE macro for checking size of size-sensitive classes (Closed)
|
Created:
4 years, 4 months ago by sashab Modified:
4 years, 3 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-wtf_chromium.org, chromium-reviews, dglazkov+blink, Mikhail, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded ASSERT_SIZE macro for checking size of size-sensitive classes
Added ASSERT_SIZE macro for checking size of size-sensitive classes,
which can be used with the SameSizeAs structs to give better compile
errors with both the expected and actual size values. Also added the
assert to the check for SameSizeAsCSSValue.
Patch written with help by timloh@.
BUG=486252
Committed: https://crrev.com/d4b40c0933c8019b196097ceaeb2099498f5f0a9
Cr-Commit-Position: refs/heads/master@{#417540}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebased & changed macro to check size only #Patch Set 3 : g cl try #
Total comments: 2
Patch Set 4 : Review feedback #
Dependent Patchsets: Messages
Total messages: 31 (16 generated)
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can replace the SameSizeAs structs and better document the known size of classes. This can also be used to monitor changes to the size of more complex classes, such as subclasses as CSSValue, that would otherwise be cumbersome to write a matching SameSizeAs struct for. BUG=486252 ========== to ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can replace the SameSizeAs structs and better document the known size of classes. This can also be used to monitor changes to the size of more complex classes, such as subclasses as CSSValue, that would otherwise be cumbersome to write a matching SameSizeAs struct for. Also replaced the SameSizeAs struct in CSSValue with the new assert. BUG=486252 ==========
sashab@chromium.org changed reviewers: + esprehn@chromium.org, timloh@chromium.org
Description was changed from ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can replace the SameSizeAs structs and better document the known size of classes. This can also be used to monitor changes to the size of more complex classes, such as subclasses as CSSValue, that would otherwise be cumbersome to write a matching SameSizeAs struct for. Also replaced the SameSizeAs struct in CSSValue with the new assert. BUG=486252 ========== to ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can replace the SameSizeAs structs and better document the known size of classes. This can also be used to monitor changes to the size of more complex classes, such as subclasses as CSSValue, that would otherwise be cumbersome to write a matching SameSizeAs struct for. Also replaced the SameSizeAs struct in CSSValue with the new assert. Patch originally written by timloh@. BUG=486252 ==========
Description was changed from ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can replace the SameSizeAs structs and better document the known size of classes. This can also be used to monitor changes to the size of more complex classes, such as subclasses as CSSValue, that would otherwise be cumbersome to write a matching SameSizeAs struct for. Also replaced the SameSizeAs struct in CSSValue with the new assert. BUG=486252 ========== to ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can replace the SameSizeAs structs and better document the known size of classes. This can also be used to monitor changes to the size of more complex classes, such as subclasses as CSSValue, that would otherwise be cumbersome to write a matching SameSizeAs struct for. Also replaced the SameSizeAs struct in CSSValue with the new assert. Patch originally written by timloh@. BUG=486252 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
I'm pretty skeptical of this, it makes it hard to understand where the space is going in a way that the SameSizeAs structs show you, ex. 5 ptrs + 1 bool. In either case this can't be conditional on the ASSERT setting, we always turn them on for the CQ , only the waterfall has them disabled. So breaking patches would always sneak through. https://codereview.chromium.org/2231953003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/SizeAssertions.h (right): https://codereview.chromium.org/2231953003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/SizeAssertions.h:27: #if ENABLE(ASSERT) The bots and CQ always enable asserts, so this means you're never checking sizes in the CQ. I don't think we want that.
On 2016/08/11 09:47:40, esprehn wrote: > I'm pretty skeptical of this, it makes it hard to understand where the space is > going in a way that the SameSizeAs structs show you, ex. 5 ptrs + 1 bool. Some context, since this was originally my patch (see https://codereview.chromium.org/1317143005/): I wanted to replace all the SameSizeAs structs with this sort of assert. IIRC these don't work with RefCounted objects because of extra fields in RefCounted added dependent on flags, but GarbageCollected(Finalized) doesn't do this. I think my reasons for wanting to do this were: - Any of the SameSizeAs structs with non-primitive types are hard to see how big they actually are unless you know all the different member types. I generally want to see the actual numbers when the SameSizeAs change structs are changed -- I think this is more useful to know than what the types are. - It's harder to see the effect of improving any of the types used (e.g. maybe we'll be able to shrink Length from 8B to 4B at one point?) - It's harder to see the effect of improving packing when the SameSizeAs just copies the types (I don't think this happens much but SameSizeAsStyleRareInheritedData is pretty bad) > In either case this can't be conditional on the ASSERT setting, we always turn > them on for the CQ , only the waterfall has them disabled. So breaking patches > would always sneak through. Yep :)
What's the status here?
Wdyt about this patch? If I can get it to work with DEBUG fields would you be interested? I prefer it over the same size as structs simply because the error messages are more meaningful (tells you the actual size, not just that the assert failed). Could we have a compromise like using the structs for size but still having a custom assert? Eg ASSERT_SIZE_EQ(SameSizeAsCSSValue, CSSValue) That gives us the better error messages but still let's you define your fields etc and see where the storage is going. On 31 Aug 2016 9:37 AM, <esprehn@chromium.org> wrote: > > What's the status here? > > https://codereview.chromium.org/2231953003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Wdyt about this patch? If I can get it to work with DEBUG fields would you be interested? I prefer it over the same size as structs simply because the error messages are more meaningful (tells you the actual size, not just that the assert failed). Could we have a compromise like using the structs for size but still having a custom assert? Eg ASSERT_SIZE_EQ(SameSizeAsCSSValue, CSSValue) That gives us the better error messages but still let's you define your fields etc and see where the storage is going. On 31 Aug 2016 9:37 AM, <esprehn@chromium.org> wrote: > > What's the status here? > > https://codereview.chromium.org/2231953003/ -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Another reason I don't like the structs is they're not consistent and there's no standard on how to write them, but maybe that's not important. On 31 Aug 2016 11:46 AM, "Sasha Bermeister" <sashab@chromium.org> wrote: > Wdyt about this patch? If I can get it to work with DEBUG fields would you > be interested? I prefer it over the same size as structs simply because the > error messages are more meaningful (tells you the actual size, not just > that the assert failed). > > Could we have a compromise like using the structs for size but still > having a custom assert? Eg > > ASSERT_SIZE_EQ(SameSizeAsCSSValue, CSSValue) > > That gives us the better error messages but still let's you define your > fields etc and see where the storage is going. > > On 31 Aug 2016 9:37 AM, <esprehn@chromium.org> wrote: > > > > What's the status here? > > > > https://codereview.chromium.org/2231953003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Another reason I don't like the structs is they're not consistent and there's no standard on how to write them, but maybe that's not important. On 31 Aug 2016 11:46 AM, "Sasha Bermeister" <sashab@chromium.org> wrote: > Wdyt about this patch? If I can get it to work with DEBUG fields would you > be interested? I prefer it over the same size as structs simply because the > error messages are more meaningful (tells you the actual size, not just > that the assert failed). > > Could we have a compromise like using the structs for size but still > having a custom assert? Eg > > ASSERT_SIZE_EQ(SameSizeAsCSSValue, CSSValue) > > That gives us the better error messages but still let's you define your > fields etc and see where the storage is going. > > On 31 Aug 2016 9:37 AM, <esprehn@chromium.org> wrote: > > > > What's the status here? > > > > https://codereview.chromium.org/2231953003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, I updated it to take 2 structs instead and compare the size, producing an
error message like so if they don't match:
In file included from ../../third_party/WebKit/Source/core/css/CSSValue.cpp:65:
../../third_party/WebKit/Source/wtf/SizeAssertions.h:14:9: error: static_assert
failed "Class should stay small"
static_assert(ActualSize == ExpectedSize, "Class should stay small");
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/wtf/SizeAssertions.h:17:31: note: in
instantiation of template class 'WTF::assert_size<blink::CSSValue,
blink::SameSizeAsCSSValue>::assertSizeEqual<4, 8>' requested here
static const bool value = assertSizeEqual<sizeof(T), sizeof(U)>::innerValue;
^
../../third_party/WebKit/Source/core/css/CSSValue.cpp:73:1: note: in
instantiation of template class 'WTF::assert_size<blink::CSSValue,
blink::SameSizeAsCSSValue>' requested here
ASSERT_SIZE(CSSValue, SameSizeAsCSSValue);
^
../../third_party/WebKit/Source/wtf/SizeAssertions.h:23:24: note: expanded from
macro 'ASSERT_SIZE'
static_assert(WTF::assert_size<className, sameSizeAsClassName>::value, "");
^
1 error generated.
It's not great, but it's probably the best error message we can get. I tried
moving all the code to inside the macro to try and customize the error message,
but static_assert doesn't let you do normal macro concatenation (className
"should stay small" gives the error "expected string literal for diagnostic
message in static_assert").
But this is still more informative than what we currently have, e.g. on a
failing trybot I've seen:
../../third_party/WebKit/Source/core/style/ComputedStyle.cpp:88:1: error: static
assertion failed: ComputedStyle should stay small
static_assert(sizeof(ComputedStyle) == sizeof(SameSizeAsComputedStyle),
"ComputedStyle should stay small");
^
With no information on whether ComputedStyle is too big or too small, and by how
much, etc.
lgtm https://codereview.chromium.org/2231953003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/SizeAssertions.h (right): https://codereview.chromium.org/2231953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/SizeAssertions.h:1: #ifndef WTF_SizeAssertions_h missing the short copyright header
Also I think your change description needs updating?
Description was changed from ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can replace the SameSizeAs structs and better document the known size of classes. This can also be used to monitor changes to the size of more complex classes, such as subclasses as CSSValue, that would otherwise be cumbersome to write a matching SameSizeAs struct for. Also replaced the SameSizeAs struct in CSSValue with the new assert. Patch originally written by timloh@. BUG=486252 ========== to ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can be used with the SameSizeAs structs to give better compile errors with both the expected and actual size values. Also added the assert to the check for SameSizeAsCSSValue. Patch written with help by timloh@. BUG=486252 ==========
The CQ bit was checked by sashab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done, thanks so much!! :D https://codereview.chromium.org/2231953003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/SizeAssertions.h (right): https://codereview.chromium.org/2231953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/SizeAssertions.h:1: #ifndef WTF_SizeAssertions_h On 2016/09/08 at 22:25:28, esprehn wrote: > missing the short copyright header Thanks! Also removed unused #include below. Still bummed we can't have compile error messages like "CSSValue is %d bytes, expected it to be %d bytes" It's too bad static_assert doesn't allow parametrized strings :'(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sashab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2231953003/#ps60001 (title: "Review feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can be used with the SameSizeAs structs to give better compile errors with both the expected and actual size values. Also added the assert to the check for SameSizeAsCSSValue. Patch written with help by timloh@. BUG=486252 ========== to ========== Added ASSERT_SIZE macro for checking size of size-sensitive classes Added ASSERT_SIZE macro for checking size of size-sensitive classes, which can be used with the SameSizeAs structs to give better compile errors with both the expected and actual size values. Also added the assert to the check for SameSizeAsCSSValue. Patch written with help by timloh@. BUG=486252 Committed: https://crrev.com/d4b40c0933c8019b196097ceaeb2099498f5f0a9 Cr-Commit-Position: refs/heads/master@{#417540} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d4b40c0933c8019b196097ceaeb2099498f5f0a9 Cr-Commit-Position: refs/heads/master@{#417540} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
