Differential D61139
(ABI break) Remove could-be-defaulted SMFs from `stack` and `queue` and `priority_queue`. Authored by • Quuxplusone on Apr 25 2019, 10:23 AM.
Details
This is an ABI break iff the defaulted SMF becomes trivial; for example, std::stack<char, trivial_fixed_capacity_vector<char, 8>> is trivially copyable after this change, which affects the calling convention when passing it by value. Also, std::stack<T, fixed_capacity_vector<T, N>> is nothrow-copy-constructible after this change; that's not an ABI break but it is detectable.
Diff Detail
Event TimelineComment Actions
Benefit: maintainability (less code) and micro performance (defaulted SMFs are easier to optimize) and macro performance (defaulted SMFs are conditionally trivial/noexcept/constexpr whereas non-defaulted ones aren't). There are no trivial container types yet in C++17, but my understanding is that C++2a (or at most, C++2b) is likely to support fixed_capacity_vector under the name std::static_vector, and that type will be conditionally trivial. So if libc++ were ever going to take an ABI break to gain trivial/noexcept/constexpr/defaulted SMFs, libc++ might want to do it sooner rather than later. Comment Actions Could we remove the code while making sure that we keep queue & friends non-trivial even when the underlying container is trivial? Something like: // 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
};It seems like this achieves the best of both world: the implementation looks nicer, we preserve the ABI (unless you don't care), and we document the whole thing nicely. What do you think? Comment Actions [snip]
I think we don't get any maintainability benefit; if anything, the code is now more complicated.
None of the standard containers are trivial. We have no idea about user-defined containers. I'm not (necessarily) saying "don't do this"; but I don't think we've thought this through yet. Comment Actions I somewhat agree, but what I meant is there's less code (namely we get rid of the SMFs). I'm neutral about this change provided we don't break the ABI (otherwise my stance is obvious). Comment Actions @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 completely gave up on my original ABI-breaking perf-improvement idea, and just eliminated all the SMFs except a simple ~queue() {} /* deliberately not =default'ed */ to ward off the perf improvement... I do think that would be simpler. But you'd have to =default all the other SMFs anyway, because user-providing the destructor causes the move operations to disappear. 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. :( Comment Actions 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.
Are you sure about that? cppreference says:
In that case, the destructor would be explicitly defined as defaulted on its first declaration (and all the other bullets are satisfied too). Comment Actions 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." Comment Actions But that's already a code path we need to test, since it's the ABI v2. So it's not adding a new configuration we need to test, it's just changing a code path we should already be testing.
Yes, you're right. I thought my snippet said: #if defined(_LIBCPP_ABI_CONTAINER_ADAPTERS_ALWAYS_NONTRIVIAL) ~queue() = default; #endif but I was mistaken, and this wouldn't have the intended effect anyways. |