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

augment libc++ test for smart pointers #18991

Merged
merged 2 commits into from
Oct 30, 2019

Conversation

pabigot
Copy link
Collaborator

@pabigot pabigot commented Sep 7, 2019

Confirms behavior of new and delete using best-practices API.

This also requires a change to the testsuite to use the correct type for the condition argument.

@pabigot pabigot added area: Test Framework Issues related not to a particular test, but to the framework instead area: C++ labels Sep 7, 2019
@pabigot pabigot requested a review from vanwinkeljan September 7, 2019 12:33
@pabigot pabigot requested a review from nashif as a code owner September 7, 2019 12:33
@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Sep 7, 2019
zassert_equal(make_unique_data::ctors, 0, "ctor count not initialized");
zassert_equal(make_unique_data::dtors, 0, "dtor count not initialized");
auto d = std::make_unique<make_unique_data>();
zassert_true(static_cast<bool>(d), "allocation failed");
Copy link
Member

Choose a reason for hiding this comment

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

Would it not be better to compare against nullptr instead of casting to a bool?

Comment also applies to release check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the intent was specifically to take advantage of the defined behavior for operator bool() on smart pointers (and other containers). Bare d doesn't convert to a pointer because it's a smart pointer; you have to do d.get() which is not what real code would use when checking whether the pointer was valid.

However that doesn't work for argument passing because it's not one of the cases where contextual conversion occurs, so here an explicit cast is required. It works for the release check because that code is a macro where the passed condition is !d, where the conversion does occur.

Copy link
Member

Choose a reason for hiding this comment

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

Ok missed the operator bool in the c++ docs, note that operator == is overloaded so comparing to a nullptr (or another unique pointer) is a valid operation.

Copy link
Collaborator Author

@pabigot pabigot Sep 7, 2019

Choose a reason for hiding this comment

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

Heh; and I missed that, since I don't think I'd ever use a smart pointer that way. It'd happen in templates, though, so it's good it's there.

@pabigot

This comment has been minimized.

Use of the test suite in C++ causes warnings because use of defined
cast operators have the wrong target type.  For example, many standard
container APIs use operator bool() to test for empty containers.  Code
like zassert_true(v, "") fails to build when the test parameter is an
int.  Correct the argument type.

This also causes any use of an assignment expression as a conditional
in zassert to be diagnosed as a potential error.

Signed-off-by: Peter A. Bigot <pab@pabigot.com>
… use

Confirm that std::make_unique<> functions as intended.  This is an
indirect test of new/delete using best-practices API.

Signed-off-by: Peter A. Bigot <pab@pabigot.com>
@pabigot pabigot removed the Blocked Blocked by another PR or issue label Oct 29, 2019
@galak galak merged commit 4ccf4f4 into zephyrproject-rtos:master Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C++ area: Test Framework Issues related not to a particular test, but to the framework instead area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants