[libcxx-commits] [PATCH] D142902: [libcxx][pmr] Make std::pmr::memory_resource ABI-compatible with GNU libstdc++ and Microsoft STL implementation.

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun added a comment.

> I'm not convinced it's a good idea to guarantee ABI compatibility in any way here, but my main concern right now is that the changes in this patch don't reflect what is claimed in the commit message. `monotonic_buffer_resource`, etc. are clearly not ABI compatible and `polymorphic_resource` should probably also be in the versioned namespace. I see no reason to change that.

Ok, I basically agree that we should not mix things. But I do believe that making `std::pmr::<get|set>_default_resourse` as well as `std::pmr::memory_resource` itself still worth our efforts because as I said it's basic global API for default memory allocation source and might be used application-wide. I definitely agree that `monotonic_buffer_resource` (+ other resources) and `polymorphic_allocator` (I believe, you meant that) are not ABI compatible and I am not arguing they should be. Unfortunately, we can not mix `namespace std::pmr` and `namespace std::_1::pmr` where `_1` is `inline namespace` because in that case `pmr` name becomes ambiguous (https://godbolt.org/z/YPEY6Kd8o). What we could do to prevent that ambiguity is

either (Actually I don't believe anybody would want that because it's not consistent with the rest of the library implementation and is not something how it is supposed to introduce the versioning namespace):

  namespace std {
  namespace pmr
  {
  inline namespace _1 {
  // Non-ABI compatible pmr stuff
  }
  }

Or something like (That option is more viable in my opinion):

  namespace std {
  
      namespace pmr {
          class memory_resource {};
      }
  
      inline namespace _1 {
          namespace detail { // whatever name you prefer
              // and other non ABI-compatible APIs
              class monotonic_buffer_resource {};
          }
      }
  
      namespace pmr {
          // and other pmr non ABI-compatible API names
          using detail::monotonic_buffer_resource;
      }  
  }

> The aliases also don't belong in the unversioned namespace. It doesn't make any real difference, but you look twice every time you come across it.

I completely agree with that. They just don't affect ABI anyhow. So, I don't care in which namespace they are. The reason why it was changed is exactly because of `pmr` name ambiguity issue I am talking about above.

> (BTW it would have been nice to see this patch a few months ago, not after the release branch where the ABI is set in stone to have some time to discuss this)

Sorry about that but I occasionally found out that `<memory_resource>` was implemented in libc++ (at the end of December IIRC) and unfortunately I am not familiar when the release branch is created and frozen. The end of December doesn't sound like "a few months ago" anyway but I do believe if we still have some opportunity window to make it happen in the upcoming release it would be great because later it would mean ABI breaking change that (I think) makes it impossible to introduce in future.

P.S. buildbot is broken with these changes right now but I want to have an understanding if we do that or not and how we do that before fixing every error in the CI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142902