[libcxx-commits] [PATCH] D99091: [locale][num_get] Improve Stage 2 of string to float conversion

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Somewhat embarrassing, but this simply went under my radar. I am aware of a couple of correctness bugs against our localization library/iostream and I am definitely interested in patches that would fix those. If you are still willing to, we can reopen and review this.

This overall LGTM, but I'd like to enable this change slightly differently. It should allow us to use the new function for all software built for a sufficiently recent deployment target. What we could do is keep the old implementation of `__stage2_float_loop` without the `bool& hex` parameter inside the dylib only and use a bit of cleverness to keep things working on older versions, like so:

  ///////////////////////////////////////////////////////////////////////////////////////////////////
  // in <__config>, add (grep for _LIBCPP_ABI_ENABLE_ADDITIONAL_IOSTREAM_EXPLICIT_INSTANTIATIONS_1):
  ///////////////////////////////////////////////////////////////////////////////////////////////////
  #if defined(_LIBCPP_BUILDING_LIBRARY) || defined(_LIBCPP_ABI_UNSTABLE) || _LIBCPP_ABI_VERSION >= 2
  // Explain what's going on
  # define _LIBCPP_ABI_LOCAL_NUM_GET_NEW_STAGE2_FLOAT_LOOP
  #endif
  
  
  ///////////////////////////////////////////////////////////////////////////////////////////////////
  // in <locale>:
  ///////////////////////////////////////////////////////////////////////////////////////////////////
  template <class _CharT>
  struct __num_get : protected __num_get_base {
  #if defined(_LIBCPP_ABI_LOCAL_NUM_GET_NEW_STAGE2_FLOAT_LOOP)
      // your new implementation
      static int __stage2_float_loop(_CharT, bool&, char&, char*, char*&, _CharT, _CharT, const string&, unsigned*, unsigned*&, unsigned&, _CharT*, bool& __hex);
  #else
      // new signature but forwarding to the old function. now you can change all the callers of this
      // function to use the new signature unconditionally, effectively making this hack more local.
      _LIBCPP_HIDE_FROM_ABI static inline 
      int __stage2_float_loop(_CharT, bool&, char&, char*, char*&, _CharT, _CharT, const string&, unsigned*, unsigned*&, unsigned&, _CharT*, bool& __hex) {
          return __stage2_float_loop(same args but drop __hex);
      }
  #endif
  
      // If we are building the dylib, we keep the old function around for backwards compatibility.
      // If we are building for a target that doesn't support the new implementation, use the old function.
  #if defined(_LIBCPP_BUILDING_LIBRARY) || !defined(_LIBCPP_ABI_LOCAL_NUM_GET_NEW_STAGE2_FLOAT_LOOP)
      static int __stage2_float_loop(_CharT, bool&, char&, char*, char*&, _CharT, _CharT, const string&, unsigned*, unsigned*&, unsigned&, _CharT*);
  #endif
  };

Benefits of this approach:

1. All callers can pretend they are using the new, fixed version.
2. The dylib retains the old function implementation, so we don't break the ABI.
3. By default, all code will keep using the old implementation, but the new implementation will also start being exported by the shared library.
4. Once the new implementation has shipped, vendors can go back and enable `_LIBCPP_ABI_LOCAL_NUM_GET_NEW_STAGE2_FLOAT_LOOP` based on whether they know the user is compiling for a target where the new implementation existed.

Concretely, this means that for example I can go back in a bit of time and enable the fix for anyone that's deploying to a recent macOS/iOS/whateverOS. The same can be done for other platforms (although I don't know that other vendors use these sorts of tricks that are available to them).

P.S.: The localization code is crazy and you're a real warrior for jumping into it -- I'm really sorry you didn't catch my attention the first time around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99091