This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Add some missing xlocale wrapper functions for OpenBSD
ClosedPublic

Authored by brad on Mar 31 2022, 8:27 PM.

Details

Summary

When updating OpenColorIO it was noticed that the function strtol_l() was missing and looking around I see strtoul_l() was also missing.

Diff Detail

Event Timeline

brad created this revision.Mar 31 2022, 8:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2022, 8:27 PM
brad requested review of this revision.Mar 31 2022, 8:27 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMar 31 2022, 8:27 PM
brad updated this revision to Diff 419612.Mar 31 2022, 8:54 PM

Formatting and style fixes.

I wonder whether this only affects OpenBSD or other platforms as well. If it also affects other platforms should these functions be here or in _support/xlocale/__strtonum_fallback.h?

libcxx/include/__support/openbsd/xlocale.h
24

Why the static inline instead of inline?
Please add _LIBCPP_HIDE_FROM_ABI.

brad added a comment.Apr 6 2022, 4:57 PM

I wonder whether this only affects OpenBSD or other platforms as well. If it also affects other platforms should these functions be here or in _support/xlocale/__strtonum_fallback.h?

I was wondering about that as well, but thought I should keep this specific to OpenBSD for now to reduce the likelihood of something breaking and can be expanded later if desired.

brad added inline comments.Apr 6 2022, 5:02 PM
libcxx/include/__support/openbsd/xlocale.h
24

Why the static inline instead of inline?
Please add _LIBCPP_HIDE_FROM_ABI.

I copied bits from libcxx/include/__support/solaris/xlocale.h, but the same goes for libcxx/include/__support/ibm/xlocale.h and libcxx/include/__support/musl/xlocale.h.

I don't see _LIBCPP_HIDE_FROM_ABI used anywhere in that sub-dir.

I wonder whether this only affects OpenBSD or other platforms as well. If it also affects other platforms should these functions be here or in _support/xlocale/__strtonum_fallback.h?

I was wondering about that as well, but thought I should keep this specific to OpenBSD for now to reduce the likelihood of something breaking and can be expanded later if desired.

Fair point.

libcxx/include/__support/openbsd/xlocale.h
24

This is used in _support/xlocale/__strtonum_fallback.h

inline _LIBCPP_INLINE_VISIBILITY long long                                      
strtoll_l(const char *nptr, char **endptr, int base, locale_t) {
  return ::strtoll(nptr, endptr, base);
}

Note that _LIBCPP_INLINE_VISIBILITY is the deprecated name for _LIBCPP_HIDE_FROM_ABI

@ldionne Do you know why these locale headers use static inline instead of inline LIBCPP_HIDE_FROM_ABI?

ldionne added inline comments.Apr 8 2022, 1:48 PM
libcxx/include/__support/openbsd/xlocale.h
24

I don't know why, but it's certainly unusual for us to define static inline functions in headers. I think this should be inline only (and _LIBCPP_HIDE_ROM_ABI like everything else).

Edit: It might be because those are meant to bridge a gap between the C headers on the system, and that's a C like way of defining those functions? Regardless, I think we should go for just inline.

brad updated this revision to Diff 421676.Apr 8 2022, 8:38 PM

Update based on feedback.

Mordante accepted this revision.Apr 21 2022, 9:11 AM

Sorry I missed your previous ping. LGTM!

This revision is now accepted and ready to land.Apr 21 2022, 9:11 AM