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

ambigulity: unknown type name 'nullptr_t' #528

Merged
merged 2 commits into from
Jul 23, 2021
Merged

Conversation

xwnb
Copy link
Contributor

@xwnb xwnb commented Jul 22, 2021

Signed-off-by: xwnb xwnbzc@outlook.com

When build ros2/demos image_tools on Ubuntu, a compile error occurred (using clang). Looks like the nullptr_t causes ambiguous between std::nullptr_t and cv::nullptr_t.

error:

error: unknown type name 'nullptr_t'

compile command:

colcon build --cmake-force-configure --symlink-install

system version:

Linux version 5.8.0-59-generic (buildd@lcy01-amd64-022) (gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #66~20.04.1-Ubuntu SMP Thu Jun 17 11:14:10 UTC 2021

clang version:

clang version 10.0.0-4ubuntu1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

Signed-off-by: xwnb <xwnbzc@outlook.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

It seems reasonable to me. According to https://en.cppreference.com/w/cpp/types/nullptr_t, nullptr_t comes from the cstddef header. Can you please add a #include <cstddef> to each of the files as well?

@xwnb
Copy link
Contributor Author

xwnb commented Jul 22, 2021

It seems reasonable to me. According to https://en.cppreference.com/w/cpp/types/nullptr_t, nullptr_t comes from the cstddef header. Can you please add a #include <cstddef> to each of the files as well?

This header is contained by other stardand header, such as #include <utility> or #include <variant>. I think we don't have to add this extra header.

@xwnb xwnb requested a review from clalancette July 22, 2021 13:13
@clalancette
Copy link
Contributor

This header is contained by other stardand header, such as #include <utility> or #include <variant>. I think we don't have to add this extra header.

While that is true, I always like to include what I'm using. You never know when a downstream header will change. So please add it in.

@clalancette
Copy link
Contributor

Can you please sign your second commit? That will allow our DCO bot to pass. Once that is done, I can run CI here. Thanks.

@xwnb
Copy link
Contributor Author

xwnb commented Jul 22, 2021

I am trying do push the new commit. But it is a connect fatal. I do not know why. Can you offer me some help?

git push --force-with-lease origin master
Connection reset by 13.250.177.223 port 22
fatal: Could not read from remote repository.

Please make sure you have the correct access rights

@clalancette
Copy link
Contributor

I am trying do push the new commit. But it is a connect fatal. I do not know why. Can you offer me some help?

Hm, I don't know, sorry. I've reset this back to the old commit, then you can try pushing without --force-with-lease to see if it is any better.

Signed-off-by: xwnb <xwnbzc@outlook.com>
@xwnb
Copy link
Contributor Author

xwnb commented Jul 22, 2021

Works. Thanks @clalancette 👍

@clalancette
Copy link
Contributor

clalancette commented Jul 22, 2021

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette clalancette merged commit 630f053 into ros2:master Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants