Add C++ version check to char16 definition #3

Acuadros95 · 2023-04-10T09:02:55Z

This definition is problematic for C++ compilers using C++11 or superior as char16_t becomes a builtin type, leading to the following error:

/__w/micro_ros_mbed/micro_ros_mbed/micro_ros_mbed/include/rosidl_dynamic_typesupport/rosidl_dynamic_typesupport/uchar.h:31:24: error: redeclaration of C++ built-in type 'char16_t' [-fpermissive]
   31 | typedef uint_least16_t char16_t;

This PR adds a check for C++ version used, avoiding the type definition for C++11 and upper versions.

pablogs9 · 2023-04-10T17:32:29Z

@methylDragon could you take a look? This is required for the upcoming micro-ROS Iron release. Thanks a lot.

emersonknapp · 2023-04-10T18:21:27Z

include/rosidl_dynamic_typesupport/uchar.h

@@ -26,7 +26,7 @@ extern "C" {
 #    define INCLUDED_UCHAR 1
 #  endif
 #endif
-#if !defined(INCLUDED_UCHAR)
+#if !defined(INCLUDED_UCHAR) && __cplusplus <= 199711L


This will only compile if __cplusplus is defined, no?

I guess that we shall also add a check on defined(__cplusplus). Can you take a look @Acuadros95?

Undefined values on macros are replaced with a 0 value. So if __cplusplus is not defined this condition will always be true: __cplusplus <= 199711L.

As @clalancette mentioned, this does not make sense. We use C++14/C++17 with gcc and msvc and this compiles fine. According to the standard, this type is defined only when <uchar.h> is included:

https://en.cppreference.com/w/c/string/multibyte/char16_t

Since this header is not available on some machines, e.g. macOS and Cygwin, we added this bit of code. I think we need some checks to avoid making this type def in more cases (like yours), but it's not because of the C++ version. It likely has to do with the std library you are using in microROS and that is defines this type (char16_t) in a different header that is being included automatically here, e.g. maybe via stdint.h or something else in the .c file that's failing to compile.

We use C++14/C++17 with gcc and msvc and this compiles fine. According to the standard, this type is defined only when <uchar.h> is included:

That is for C, for C++11 and beyond its a keyword and cannot be redefined:

https://en.cppreference.com/w/cpp/keyword/char16_t

I guess the problem is that our compiler does not have the __has_include definition, and the <uchar.h> header is most probably checking the used C++ version to add the typedef or not, example:

https://codebrowser.dev/llvm/include/uchar.h.html#34

I guess the problem is that our compiler does not have the __has_include definition, and the <uchar.h> header is most probably checking the used C++ version to add the typedef or not, example:

I see. That makes a whole lot more sense. Thanks. In that case, then I think we should probably rewrite this whole section (including the stuff above) to something like:

#if defined(__cplusplus) && __cplusplus >= 201103L // Nothing to do here, C++11 and beyond have char16_t as a keyword: // https://en.cppreference.com/w/cpp/keyword/char16_t #elif defined(__has_include) && __has_include(<uchar.h>) // If the compiler has __has_include, and uchar.h exists, include that as it will have char16_t // as a typedef. # include <uchar.h> #else // Otherwise assume that char16_t isn't defined anywhere, and define it ourselves as uint_least16_t. # include <stdint.h> typedef uint_least16_t char16_t; #endif

That is, if we are on C++11 or newer, just assume it is a keyword. If we aren't on C++, or it is too old, then try to use __has_include to find the uchar.h header and include that. If all of that fails, assume char16_t isn't defined and define it ourselves. @pablogs9 @Acuadros95 can you give this a try and see if it works with your compiler?

Sounds good. @Acuadros95 can you try it?

@clalancette Just tested the new checks and can confirm it works.

I have updated the PR here ac34007

clalancette · 2023-04-10T18:28:58Z

I'm actually a little confused about:

Why we have this typedef in the first place, but also
Why microROS is struggling with this. We compile everything with C++17/C++14, and we are not seeing this problem.

pablogs9 · 2023-04-11T05:40:39Z

Why we have this typedef in the first place, but also

If it is ok to remove it, we are also ok.

Why microROS is struggling with this. We compile everything with C++17/C++14, and we are not seeing this problem.

As far as I have tested, in GCC 12 with C++17 this kind of override is an error unless you have -fpermissive in your build flags: https://godbolt.org/z/YMvYThWbs

Could you take a look at that @clalancette ?

clalancette · 2023-04-12T17:57:05Z

As far as I have tested, in GCC 12 with C++17 this kind of override is an error unless you have -fpermissive in your build flags: https://godbolt.org/z/YMvYThWbs

Sure, but the check that is already there seems to handle it. If you edit your example to put the current checks in place, it compiles fine: https://godbolt.org/z/zYG81csjr

I also verified locally that I can build with gcc 12.2.1, and everything compiles fine here. So I still don't understand why your build is failing.

Yadunund · 2023-04-13T05:48:08Z

I encounter the same error on MacOS with Apple Clang and can confirm that this PR fixes the build.

pablogs9 · 2023-04-13T06:09:06Z

Hello, @Yadunund are you using an ARM architecture?

@clalancette here you have my local tests using ARM compiler:

Ubuntu 20.04 + gcc-arm-none-eabi 9.2.1

root@43e89bb2a4ab:~# cat main.cpp
#include <stdint.h>

#if defined __has_include
#  if __has_include(<uchar.h>)
#    include <uchar.h>
#    define INCLUDED_UCHAR 1
#  endif
#endif

#if !defined(INCLUDED_UCHAR)
#  include <stdint.h>
typedef uint_least16_t char16_t;
#endif

int main() {
    char16_t a = 'a';

    return a;
}
root@43e89bb2a4ab:~# arm-none-eabi-gcc main.cpp
main.cpp:12:24: error: redeclaration of C++ built-in type 'char16_t' [-fpermissive]
   12 | typedef uint_least16_t char16_t;
      |                        ^~~~~~~~
root@43e89bb2a4ab:~# arm-none-eabi-gcc --version
arm-none-eabi-gcc (15:9-2019-q4-0ubuntu1) 9.2.1 20191025 (release) [ARM/arm-9-branch revision 277599]
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Ubuntu 22.04 + gcc-arm-none-eabi 10.3.1

root@1c7b6e7a714b:~# cat main.cpp 
#include <stdint.h>

#if defined __has_include
#  if __has_include(<uchar.h>)
#    include <uchar.h>
#    define INCLUDED_UCHAR 1
#  endif
#endif

#if !defined(INCLUDED_UCHAR)
#  include <stdint.h>
typedef uint_least16_t char16_t;
#endif

int main() {
    char16_t a = 'a';

    return a;
}   
root@1c7b6e7a714b:~# arm-none-eabi-gcc main.cpp 
main.cpp:12:24: error: redeclaration of C++ built-in type 'char16_t' [-fpermissive]
   12 | typedef uint_least16_t char16_t;
      |                        ^~~~~~~~
root@1c7b6e7a714b:~# arm-none-eabi-gcc --version
arm-none-eabi-gcc (15:10.3-2021.07-4) 10.3.1 20210621 (release)
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Yadunund · 2023-04-13T06:12:25Z

Ah yes forgot to mention i'm on arm64 (Apple Silicon)

pablogs9 · 2023-04-19T08:06:27Z

Any update on this?

wjwwood

I don't think this is the right check to make to avoid this typedef, though I can see how some other condition should be here to avoid this problem.

wjwwood · 2023-04-19T21:12:43Z

For comparison, on my macOS machine (arm64) @pablogs9's main.cpp works fine:

wjwwood@wjwwood-macbookpro /tmp/test
% gcc --version
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: arm64-apple-darwin22.4.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

wjwwood@wjwwood-macbookpro /tmp/test
% gcc main.cpp

clalancette · 2023-07-10T12:30:35Z

Here's a few different kinds of CI, to make sure that this is still going to compile:

Linux
Linux-aarch64
Windows
Windows Debug
RHEL
Clang

Signed-off-by: acuadros95 <acuadros1995@gmail.com>

Acuadros95 · 2023-07-21T08:05:37Z

@clalancette Can this PR be backported to Iron?

clalancette · 2023-07-24T11:56:06Z

@Mergifyio backport iron

mergify · 2023-07-24T11:56:12Z

backport iron

✅ Backports have been created

#6 Add C++ version check to char16 definition (backport #3) has been created for branch iron

* Add C++ version condition to char16 definition Signed-off-by: acuadros95 <acuadros1995@gmail.com> (cherry picked from commit 0ab4827)

* Add C++ version condition to char16 definition Signed-off-by: acuadros95 <acuadros1995@gmail.com> (cherry picked from commit 0ab4827) Co-authored-by: Antonio Cuadros <49162117+Acuadros95@users.noreply.github.com>

Acuadros95 mentioned this pull request Apr 10, 2023

Update rolling build sources micro-ROS/micro_ros_mbed#36

Merged

emersonknapp reviewed Apr 10, 2023

View reviewed changes

wjwwood requested changes Apr 19, 2023

View reviewed changes

clalancette approved these changes Jul 10, 2023

View reviewed changes

Acuadros95 added 2 commits July 10, 2023 12:52

Add C++ version condition to char16 definition

b8f7fe4

Signed-off-by: acuadros95 <acuadros1995@gmail.com>

Update C++ checks approach

eeb6ecc

Signed-off-by: acuadros95 <acuadros1995@gmail.com>

clalancette merged commit 0ab4827 into ros2:rolling Jul 11, 2023
1 check passed

Acuadros95 mentioned this pull request Jul 21, 2023

Remove micro-ROS fork micro-ROS/micro_ros_platformio#110

Merged

1 task

mergify bot pushed a commit that referenced this pull request Jul 24, 2023

Add C++ version check to char16 definition (#3)

99a4f3e

* Add C++ version condition to char16 definition Signed-off-by: acuadros95 <acuadros1995@gmail.com> (cherry picked from commit 0ab4827)

mergify bot mentioned this pull request Jul 24, 2023

Add C++ version check to char16 definition (backport #3) #6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add C++ version check to char16 definition #3

Add C++ version check to char16 definition #3

Acuadros95 commented Apr 10, 2023

pablogs9 commented Apr 10, 2023

emersonknapp Apr 10, 2023

pablogs9 Apr 10, 2023

Acuadros95 Apr 11, 2023 •

edited

Loading

wjwwood Apr 19, 2023

Acuadros95 Apr 20, 2023

clalancette Jul 5, 2023

pablogs9 Jul 5, 2023

Acuadros95 Jul 10, 2023

clalancette commented Apr 10, 2023

pablogs9 commented Apr 11, 2023

clalancette commented Apr 12, 2023

Yadunund commented Apr 13, 2023

pablogs9 commented Apr 13, 2023

Yadunund commented Apr 13, 2023

pablogs9 commented Apr 19, 2023

wjwwood left a comment

wjwwood commented Apr 19, 2023

clalancette commented Jul 10, 2023 •

edited

Loading

Acuadros95 commented Jul 21, 2023

clalancette commented Jul 24, 2023

mergify bot commented Jul 24, 2023 •

edited

Loading

Add C++ version check to char16 definition #3

Add C++ version check to char16 definition #3

Conversation

Acuadros95 commented Apr 10, 2023

pablogs9 commented Apr 10, 2023

emersonknapp Apr 10, 2023

Choose a reason for hiding this comment

pablogs9 Apr 10, 2023

Choose a reason for hiding this comment

Acuadros95 Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

wjwwood Apr 19, 2023

Choose a reason for hiding this comment

Acuadros95 Apr 20, 2023

Choose a reason for hiding this comment

clalancette Jul 5, 2023

Choose a reason for hiding this comment

pablogs9 Jul 5, 2023

Choose a reason for hiding this comment

Acuadros95 Jul 10, 2023

Choose a reason for hiding this comment

clalancette commented Apr 10, 2023

pablogs9 commented Apr 11, 2023

clalancette commented Apr 12, 2023

Yadunund commented Apr 13, 2023

pablogs9 commented Apr 13, 2023

Yadunund commented Apr 13, 2023

pablogs9 commented Apr 19, 2023

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood commented Apr 19, 2023

clalancette commented Jul 10, 2023 • edited Loading

Acuadros95 commented Jul 21, 2023

clalancette commented Jul 24, 2023

mergify bot commented Jul 24, 2023 • edited Loading

✅ Backports have been created

Acuadros95 Apr 11, 2023 •

edited

Loading

clalancette commented Jul 10, 2023 •

edited

Loading

mergify bot commented Jul 24, 2023 •

edited

Loading