Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dart::CidRange marked ZoneAllocated, but used with MallocGrowableArray #36955

Closed
mdempsky opened this issue May 13, 2019 · 1 comment

Comments

Projects
None yet
3 participants
@mdempsky
Copy link
Member

commented May 13, 2019

Since dart::Malloc::Realloc uses realloc to reallocate and move constructed objects around, I thought to add a static_assert to check that T satisfies std::is_trivially_copyable.

This exposed that dart::CidRange is marked ZoneAllocated, but is used with MallocGrowableArray. I.e., it's marked ZoneAllocated, but then it's allocated in some cases on the C/C++ heap.

This seems unusual to me and at least counter to ZoneAllocated's intention with disallowing normal new/delete and the copy/assignment operators, but I can't see anything immediately wrong with it.

Is there anything that can/should be done here? Maybe just replace MallocGrowableArray with ZoneGrowableArray?

/cc @mkustermann @crelier @mraleph

@mit-mit mit-mit added the area-vm label May 14, 2019

@mraleph

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

I think this is completely intentional. At some point ZoneAllocated had an assertion in destructor preventing us from using ZoneAllocated classes from being used as normal values (e.g. allocating instances of those classes on the stack). However we removed it - because it was making code harder to write (e.g. you were forced to create a value class CidRange and then have another class ZoneCidRange which would contain CidRange inside).

I view inheriting from ZoneAllocated just a convenient way to provide default implementation of operator new which allocates in the zone - rather than a guarantee that instance of this class must always reside in the zone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.