jbroman
2016/06/02 20:20:21
I guess we're assuming here that domNodeId < (1 <<
I guess we're assuming here that domNodeId < (1 << 32UL)? Otherwise the failure
mode here seems pretty confusing.
If you can assume this statically (i.e. if we have no platforms where
sizeof(int) > sizeof(int32_t)), a static_assert here would be great in
explaining on what basis we assume this to be safe, i.e.
static_assert(sizeof(domNodeId) <= 4, "DOM node IDs are assumed not to be larger
than 32 bits");
If that's not the case, a comment and/or DCHECK would be appreciated.
Ian Vollick
2016/06/03 19:31:21
This is indeed dodgy. I've changed the innards of
On 2016/06/02 20:20:21, jbroman wrote:
> I guess we're assuming here that domNodeId < (1 << 32UL)? Otherwise the
failure
> mode here seems pretty confusing.
>
> If you can assume this statically (i.e. if we have no platforms where
> sizeof(int) > sizeof(int32_t)), a static_assert here would be great in
> explaining on what basis we assume this to be safe, i.e.
>
> static_assert(sizeof(domNodeId) <= 4, "DOM node IDs are assumed not to be
larger
> than 32 bits");
>
> If that's not the case, a comment and/or DCHECK would be appreciated.
This is indeed dodgy. I've changed the innards of ElementIds to be two ints to
avoid problems like this.
12 value |= static_cast<uint64_t>(subElementId) << 32UL;
Issue 1973083002: Use element id's for animations
(Closed)
Created 4 years, 7 months ago by Ian Vollick
Modified 4 years, 5 months ago
Reviewers: ajuma, bokan, jbroman, loyso (OOO), Rick Byers
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 52