This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Complete refactor of tests for operator new
ClosedPublic

Authored by ldionne on May 11 2023, 4:57 PM.

Details

Reviewers
philnik
Group Reviewers
Restricted Project
Commits
rG54e3fc3563b9: [libc++] Complete refactor of tests for operator new
Summary

I stumbled upon the operator new and operator new[] tests while
investigating an issue with operator new when exceptions are disabled,
and I realized that our test coverage was incomplete. This patch refactors
all the operator new and operator new[] tests to add consistency and
better coverage for scenarios in which it should be possible to override
an operator indirectly by defining another one (for example new(size_t, nothrow)
should use new(size_t) if it has been provided).

This is intended to be a NFC setting up the terrain for some refactoring
work and bug fix in operator new.

Diff Detail

Event Timeline

ldionne created this revision.May 11 2023, 4:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 4:57 PM
ldionne requested review of this revision.May 11 2023, 4:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 4:57 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added inline comments.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size.pass.cpp
27
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size.replace.indirect.pass.cpp
29

Are you allowed to override only operator new or operator delete? It yes, we might want to add a test for that.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp
37

Or is this required?

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp
59

Do we ever try calling the align_val_t overload explicitly?

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size.verify.cpp
0

We have this as an extension now, so we might as well enable this test for libc++ in all language versions.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.verify.cpp
0

Is there a reason the [[nodiscard]] tests aren't merged?

ldionne updated this revision to Diff 521623.May 12 2023, 5:54 AM

Fix C++03

ldionne marked 6 inline comments as done.May 12 2023, 6:51 AM
ldionne added inline comments.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size.replace.indirect.pass.cpp
29

I don't think so, because the memory allocated with one has to be deallocated with the other. They need to use a mechanism that matches. For example, if your custom operator new uses malloc and the system-provided operator delete happens to use free, that's fine. But that might not be the case in general, the exact mechanism isn't specified.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.indirect.pass.cpp
37

This is required in C++03 for some tests, but I removed it for others. Same for TEST_NOEXCEPT on operator delete, I replaced it with noexcept when I could.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size_align.replace.pass.cpp
59

I added some tests to explicitly call operator new for all variants.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.verify.cpp
0

Thanks for the suggestion, I just didn't think of it but it's much better indeed.

ldionne updated this revision to Diff 521638.May 12 2023, 6:52 AM
ldionne marked 4 inline comments as done.

Address comments.

philnik accepted this revision.May 15 2023, 2:22 PM

LGTM % possible test case and green CI.

libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new.size.replace.indirect.pass.cpp
29

Yes, but a user could define operator new, operator delete and operator new[]. There would still be a mismatch, but the user knows how new and delete allocate memory.

This revision is now accepted and ready to land.May 15 2023, 2:22 PM
ldionne updated this revision to Diff 523479.May 18 2023, 11:44 AM

Poke CI again.

ldionne updated this revision to Diff 524551.May 22 2023, 6:41 PM
ldionne added a subscriber: mstorsjo.

Add missing XFAILs.

@mstorsjo it would be awesome if you (or someone else with easy Windows access) could help with these new tests.

ldionne updated this revision to Diff 524797.May 23 2023, 10:31 AM

XFAIL => UNSUPPORTED

This revision was automatically updated to reflect the committed changes.
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.single/new_replace.pass.cpp