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

Use sizeof(char) in place for sizeof(void) #515

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Apr 29, 2021

The latter is invalid C++, even though gcc accepts it as an extension. Connected to ros2/rclcpp#1647.

CI up to rclcpp and demo_nodes_cpp:

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

The latter is invalid C++, even though gcc accepts it as an extension

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@clalancette
Copy link
Contributor

The latter is invalid C++, even though gcc accepts it as an extension. Connected to ros2/rclcpp#1647.

I'm not against making this change, but this can't just be a GCC extension, right? This same code is compiling with clang and MSVC as well.

@wjwwood
Copy link
Member

wjwwood commented Apr 29, 2021

Yeah, I dunno, libcxx (which works on windows afaik) just straight does it:

https://github.com/llvm/llvm-project/blob/5bf2ef9d869ba882480232a4ed87728af74dac3b/libcxx/include/__memory/allocator.h#L74-L84

There is a specialization for void, but it doesn't affect that method, afaict:

https://github.com/llvm/llvm-project/blob/aaf026d9da3885a951dcdc5edd64c8e7d23b6285/libcxx/include/__memory/allocator.h#L31-L53

@ivanpauno
Copy link
Member

I'm not against making this change, but this can't just be a GCC extension, right? This same code is compiling with clang and MSVC as well.

This seems to be a broadly accepted extension 😂.

But AFAIU it's not supposed to be correct.
According to cppreference:

sizeof cannot be used with function types, incomplete types (including void), or bit field lvalues.

@ivanpauno ivanpauno merged commit c3769dd into master Apr 30, 2021
@delete-merged-branch delete-merged-branch bot deleted the hidmic/avoid-sizeof-void branch April 30, 2021 11:46
@hidmic
Copy link
Contributor Author

hidmic commented May 3, 2021

FWIW, besides sizeof(void) being accepted by some compilers, I suspect templates' lazy instantiation has a role here. IIUC if the allocate method in an allocator bound to void is never called or even addressed, its definition won't ever be instantiated (referred to as partial instantiation in C++ Templates - The Complete Guide, section 14.2). That changed with ros2/rclcpp#1647.

hidmic added a commit that referenced this pull request May 5, 2021
hidmic added a commit that referenced this pull request May 5, 2021
This reverts commit c3769dd.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
hidmic added a commit that referenced this pull request May 10, 2021
This reverts commit c3769dd.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
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

4 participants