Fix uchar.h / char16_t detection/handling (fix #8)
#10
Conversation
Fix incorrect check: if `__has_include(..)` was supported, but `uchar.h` did not exist, `char16_t` never got `typedef`ed. It needs to be `typedef`ed in (at least) two cases: 1. `uchar.h` does not exist, and 2. `__has_include(..)` is not supported In case of the former, the type is most likely missing. In case of the latter, `uchar.h` could exist, but existence can't be checked. Note: checking for `__has_include(..)` support and use of the macro in a combined conditional is non-portable according to the documentation, hence the split.
|
It would be great to get this merged - bug from #8 currently breaks CI in a project. |
|
CI:
|
|
All right, CI is green. I'm going to go ahead and merge this, but I'm going to hold off on attempting a backport to see if we get any other reports (positive or negative) about this change. |
|
I cannot launch a new nightly from my mobile, so I relaunch yesterday's: https://github.com/micro-ROS/micro_ros_setup/actions/runs/6007791161 If this is green, I'm ok with the backport. BTW: I will check code in detail next week but, is there a real need of using char16_t? Why not using the wide supported uint16_t? |
|
@pablogs9: jobs seem to all have succeeded. The just finished nightly build also seems to be green.
tbh I don't know. I was just 'here' to fix a build issue. Edit: looks like |
Because we are using this for handling wstring, which is explicitly char16_t. My understanding is that char16_t may or may not be equivalent to uint16_t, e.g. it might be signed depending on the platform. |
|
In that case it makes sense... Thanks a lot for taking care of this @gavanderhoorn @clalancette. As CI is green for micro-ROS in rolling, for me it is ok to backport to iron. |
I would very much appreciate a backport indeed. |
Seeing as it's been a week without any additional reports: could this be backported to |
Fix incorrect check: if `__has_include(..)` was supported, but `uchar.h` did not exist, `char16_t` never got `typedef`ed. It needs to be `typedef`ed in (at least) two cases: 1. `uchar.h` does not exist, and 2. `__has_include(..)` is not supported In case of the former, the type is most likely missing. In case of the latter, `uchar.h` could exist, but existence can't be checked. Note: checking for `__has_include(..)` support and use of the macro in a combined conditional is non-portable according to the documentation, hence the split.
* uchar: use __has_include(..) on separate line (#8) Older compilers/preprocessors don't always support/implement short-circuit conditional evaluation, leading to attempts to evaluate `__has_include(..)` while `if defined(__has_include)` was actually `false`. Split the conditional into two separate checks to avoid this. (cherry picked from commit 88e2d7d) * uchar: fix conditional include/typedef (#10) Fix incorrect check: if `__has_include(..)` was supported, but `uchar.h` did not exist, `char16_t` never got `typedef`ed. It needs to be `typedef`ed in (at least) two cases: 1. `uchar.h` does not exist, and 2. `__has_include(..)` is not supported In case of the former, the type is most likely missing. In case of the latter, `uchar.h` could exist, but existence can't be checked. Note: checking for `__has_include(..)` support and use of the macro in a combined conditional is non-portable according to the documentation, hence the split. --------- Co-authored-by: G.A. vd. Hoorn <g.a.vanderhoorn@tudelft.nl>
Fix incorrect check: if
__has_include(..)was supported, butuchar.hdid not exist,char16_tnever gottypedefed with the changes to the conditionals made by #8.It needs to be
typedefed in (at least) two cases:uchar.hdoes not exist, and__has_include(..)is not supportedIn case of the former, the type is most likely missing. In case of the latter,
uchar.hcould exist, but existence can't be checked, so the code is conservative and justtypedefschar16_titself.