Skip to content
New issue

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

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

Already on GitHub? # to your account

adding few more c++17 operators override. #581

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

devnexen
Copy link
Collaborator

No description provided.

@@ -71,3 +71,45 @@ void operator delete[](void* p, std::nothrow_t&)
{
ThreadAlloc::get().dealloc(p);
}

void* operator new(size_t size, std::align_val_t)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discarding the alignment requirement seems dubious? While it's generally true that snmalloc prefer to align things much more strongly than generally required, it's still possible to ask for yet stronger alignment. I think the correct thing to do is to use

aligned_size(size_t alignment, size_t size)
as done with
SNMALLOC_EXPORT void*
SNMALLOC_NAME_MANGLE(memalign)(size_t alignment, size_t size)
{
if ((alignment == 0) || (alignment == size_t(-1)))
{
errno = EINVAL;
return nullptr;
}
if ((size + alignment) < size)
{
errno = ENOMEM;
return nullptr;
}
return SNMALLOC_NAME_MANGLE(malloc)(aligned_size(alignment, size));
}

I do not know to what extent those checks are also required by C++ .

return ThreadAlloc::get().alloc(size);
}

void operator delete(void* p, std::align_val_t)EXCEPTSPEC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void operator delete(void* p, std::align_val_t)EXCEPTSPEC
void operator delete(void* p, std::align_val_t) EXCEPTSPEC

I'm surprised clang-format was OK with that without the whitespace.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... oh wow, it in fact requires that it not be there. That... that sure seems like a bug, but I guess it's best to do what it wants. Sorry!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nwf-msr sorry I am confused now. I think @devnexen has now added the space, but are you saying the space shouldn't be there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by the way is clang-format9 still recommended ? Thankfully I still have it somewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, but I might move this and Verona forward to a newer version. It is getting a bit long in the tooth now.

@mjp41
Copy link
Member

mjp41 commented Jan 30, 2023

@devnexen would be able to run clang-format to unblock the CI?

@mjp41 mjp41 dismissed nwf-msr’s stale review January 31, 2023 09:22

Changes have been addressed from this review and @nwf-msr is OOF at the moment.

@mjp41 mjp41 merged commit 6c27b59 into microsoft:main Jan 31, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants