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

tests: fix NULL references to be acceptable by Clang #12880

Merged
merged 1 commit into from Jan 12, 2017

Conversation

Projects
None yet
3 participants
@wjwithagen
Contributor

wjwithagen commented Jan 11, 2017

  • On some platforms NULL evaluates to nullptr, which will require a
    cast to the right type to be able to compile.
    error: reinterpret_cast from 'nullptr_t' to 'ContextWQ *'

  • It is a delicate change since otherwise GCC will start complaining
    about the reverse:
    error: invalid static_cast from type 'const long int' to type 'ContextWQ*'

By casting right at the instance of NULL both compilers are happy.

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

@@ -96,7 +96,7 @@ class TestMockExclusiveLockReleaseRequest : public TestMockFixture {
int r) {
if (mock_image_ctx.object_cacher != nullptr) {
EXPECT_CALL(mock_image_ctx, invalidate_cache(purge, _))
.WillOnce(WithArg<1>(CompleteContext(r, NULL)));
.WillOnce(WithArg<1>(CompleteContext(r, static_cast<ContextWQ*>(NULL))));

This comment has been minimized.

@tchaikov

tchaikov Jan 11, 2017

Contributor

nit, maybe we can take this chance to s/NULL/nullptr/ here.

This comment has been minimized.

@wjwithagen

wjwithagen Jan 11, 2017

Contributor

@tchaikov
Perhaps in a second PR?
Or rather in this one?

This comment has been minimized.

@tchaikov

tchaikov Jan 11, 2017

Contributor

it's more C++ish, and cosmetic in this case . and i'd suggest do it in this commit.

@tchaikov tchaikov changed the title from Clang/nullptr: fix NULL references to be acceptable by Clang to tests: fix NULL references to be acceptable by Clang Jan 11, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 11, 2017

@wjwithagen might want to use "tests" as the prefix of the title of your commit instead.

@tchaikov tchaikov requested a review from dillaman Jan 11, 2017

expect_op_work_queue(mock_image_ctx);
}
void expect_invalidate_cache(MockImageCtx &mock_image_ctx, int r) {
EXPECT_CALL(mock_image_ctx, invalidate_cache(false, _))
.WillOnce(WithArg<1>(CompleteContext(r, NULL)));
.WillOnce(WithArg<1>(CompleteContext(r, static_cast<ContextWQ*>(NULL)));

This comment has been minimized.

@tchaikov

tchaikov Jan 11, 2017

Contributor
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/librbd/operation/test_mock_ResizeRequest.cc: In member function ‘void librbd::operation::TestMockOperationResizeRequest::expect_invalidate_cache(librbd::MockImageCtx&, int)’:
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/librbd/operation/test_mock_ResizeRequest.cc:124:90: error: expected ‘)’ before ‘;’ token
                   .WillOnce(WithArg<1>(CompleteContext(r, static_cast<ContextWQ*>(NULL)));
                                                                                          ^
[ 95%] Building CXX object src/test/librbd/CMakeFiles/unittest_librbd.dir/operation/test_mock_SnapshotProtectRequest.cc.o
make[3]: *** [src/test/librbd/CMakeFiles/unittest_librbd.dir/operation/test_mock_ResizeRequest.cc.o] Error 1

missing a ) here.

This comment has been minimized.

@wjwithagen

wjwithagen Jan 11, 2017

Contributor

@tchaikov
Yuo, thanx,
Same player shoots again.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 11, 2017

@wjwithagen might want to use "tests" as the prefix of the title of your commit instead.

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 11, 2017

@tchaikov
Just only --amended the commit....
So the jenkins run should come back with the same result.

tests: fix NULL references to be acceptable by Clang
 - On some platforms NULL evaluates to nullptr, which will require a
   cast to the right type to be able to compile.
      error: reinterpret_cast from 'nullptr_t' to 'ContextWQ *'

 - It is a delicate change since otherwise GCC will start complaining
   about the reverse:
      error: invalid static_cast from type 'const long int' to type 'ContextWQ*'

 By casting right at the instance of NULL both compilers are happy.

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 12, 2017

@tchaikov @dillaman
Any more changes required?
Or do we want to do NULL -> nullptr in this PR as well?

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jan 12, 2017

@wjwithagen I'm fine w/ keeping NULL, but @tchaikov requested that you switch to nullptr.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 12, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jan 12, 2017

@tchaikov @dillaman
This works atm.
Lets then leave it at NULL.

@dillaman dillaman merged commit 438f990 into ceph:master Jan 12, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@wjwithagen wjwithagen deleted the wjwithagen:wip-wjw-static_cast-NULL branch Jan 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment