[libcxx-commits] [PATCH] D61139: (ABI break) Remove could-be-defaulted SMFs from `stack` and `queue` and `priority_queue`.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

In D61139#1485234 <https://reviews.llvm.org/D61139#1485234>, @ldionne wrote:

> In D61139#1485072 <https://reviews.llvm.org/D61139#1485072>, @Quuxplusone wrote:
>
> > @ldionne: I think adding the macro `_LIBCPP_ABI_CONTAINER_ADAPTERS_ALWAYS_NONTRIVIAL` makes the code "more complicated" because then there will be two different codepaths in need of testing.
>
>
> If we want to make this change, we __have__ to guard it behind a macro because there's no way we're breaking libc++'s ABI v1 for that. And we should already have testers for the libc++ ABI v2 (if someone cares about it), so that shouldn't be a significant increase in complexity.
>
> > [...]
> > 
> > However! I think the problem with that idea is that it would make `std::stack<int, fixed_capacity_vector<int, 10>>` no longer trivially-destructible! Right now it's not trivially copyable but it is trivially destructible; after your idea, it would remain non-trivially copyable but it would no longer be trivially destructible. So your idea would sadly be a perf //regression//, not merely the desired perf non-improvement. :(
>
> Are you sure about that? cppreference says <https://en.cppreference.com/w/cpp/language/destructor#Trivial_destructor>:
>
> > The destructor for class T is trivial if all of the following is true:
> > 
> > - The destructor is not user-provided (meaning, it is either implicitly declared, or explicitly defined as defaulted on its first declaration)
> > - [...]
>
> In that case, the destructor would be explicitly defined as defaulted on its first declaration (and all the other bullets are satisfied too).


When I said "your idea," what I was referring to was the code you posted,

  // Henceforth I'll call this code SNIPPET 1.
  // in <__config>:
  #if _LIBCPP_ABI_V1
     // This macro controls whether container adapters are always non-trivial, regardless of whether the underlying Container is trivial or not. This impacts ABI because of how trivial types are passed as function arguments.
  #  define _LIBCPP_ABI_CONTAINER_ADAPTERS_ALWAYS_NONTRIVIAL
  #endif
  
  // in <queue>
  template <class T, class Container = deque<T>>
  class queue {
  public:
  #if defined(_LIBCPP_ABI_CONTAINER_ADAPTERS_ALWAYS_NONTRIVIAL)
    ~queue() { }
  #endif
  };

`SNIPPET 1` suffers from the problems I said: (1) it uses a config macro so now there's two codepaths to test, and (2) it changes `queue` from "conditionally trivially destructible" to "never trivially destructible."

So suppose we get rid of the macro. That gives us `SNIPPET 2`:

  // Henceforth I'll call this code SNIPPET 2.
  // in <queue>
  template <class T, class Container = deque<T>>
  class queue {
  public:
    ~queue() { }
  };

`SNIPPET 2` fixes problem (1) from above, but it still suffers from problem (2) it changes `queue` from "conditionally trivially destructible" to "never trivially destructible."


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D61139