[libcxx-commits] [PATCH] D124555: [libcxx] basic_ios<wchar_t> cannot store fill character WCHAR_MAX

[Chris Bowler via Phabricator via libcxx-commits]

cebowleratibm planned changes to this revision.
cebowleratibm added a comment.

Plan to revise the config to pick up the "fill set member as needed" mode in the config for all targets when _LIBCPP_ABI_VERSION >= 2.



================
Comment at: libcxx/include/__config:319
 
+#if defined(_AIX)
+#    define _LIBCPP_BASIC_IOS_HAS_FILL_SET 1
----------------
daltenty wrote:
> daltenty wrote:
> > What about adding an ABI guard around this whole block? Something like:
> > ```
> > #if _LIBCPP_ABI_BASIC_IOS_HAS_FILL_SET_IF_NEEDED
> > ```
> > And defining that for the unstable ABI (_LIBCPP_ABI_VERSION >= 2) in the relevant block here? That would let folks not requiring the stable ABI  on the below platforms pickup the fix automatically if needed.
> edit: or rather the reverse check:
> ```
> #ifndef _LIBCPP_ABI_BASIC_IOS_HAS_FILL_SET_IF_NEEDED
> ```

There are 3 states:

  # The fill set member always exists (AIX compat)
  # The fill set member never exists (non AIX target compat)
  # Add the fill set member only if needed. (the desirable path)

I chose a single macro _LIBCPP_BASIC_IOS_HAS_FILL_SET to represent these states.  The third state is achieved by leaving the macro undefined, which is to say: there's no force override so just do the right thing.

If I introduce another macro _LIBCPP_ABI_BASIC_IOS_HAS_FILL_SET_IF_NEEDED then the config is conflicted if both macros are defined.  I thought it better to avoid the potential for things to go wrong.

You have an excellent point that we can enable the "as needed" mode on all targets when _LIBCPP_ABI_VERSION >= 2.  If will revise the patch for this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124555/new/

https://reviews.llvm.org/D124555