[libcxx-commits] [PATCH] D82490: [libcxx] Put clang::trivial_abi on std::unique_ptr

[Richard Smith - zygoloid via Phabricator via libcxx-commits]

rsmith added a comment.

See also https://reviews.llvm.org/D63748; I'm happy to abandon that in favor of your work here.

This will need test cases. I think we should also consider whether we want to apply this to `shared_ptr` too, as D63748 <https://reviews.llvm.org/D63748>, and if so, whether we want a single configuration macro to apply `[[trivial_abi]]` to both classes, or one macro for each.



================
Comment at: libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst:39
+* Annotate the two definitions of ``std::unique_ptr``  with ``clang::trivial_abi`` attribute.
+* Put the attribuate behind a flag because this change has potential compilation and runtime breakages.
+
----------------
Typo "attribuate".


================
Comment at: libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst:44-46
+* ``std::unique_ptr`` parameters will now be destroyed by callees, rather than callers.
+  It is worth noting that destruction by callee is not unique to the use of trivial_abi attribute.
+  In most Microsoft's ABIs,   arguments are always destroyed by the callee.
----------------
This change to destroy `unique_ptr` parameters in the callee is technically non-conforming, because it changes destruction order for function parameters to an order that is observably different from any order that a correct implementation can produce. For example:

```
struct A { ~A(); };
struct B { ~B(); };
struct C { C(A, unique_ptr<B>, A) {} };
C c{{}, make_unique<B>, {}};
```

In a conforming implementation, the destruction order for `C::C`'s parameters is required to be `~A()`, `~B()`, `~A()`, but with this mode enabled, we'll instead see `~B()`, `~A()`, `~A()`. That's probably fine in practice for almost all users (and this isn't the only case in which implementations get destruction order for function parameters "wrong"), but it seems worth pointing out here. This isn't just a different behavior, it's a behavior disallowed by the standard.


================
Comment at: libcxx/docs/DesignDocs/UniquePtrTrivialAbi.rst:96
+
+ - Fix: Remove forward-declaration of ``Foo`` and include its proper headrer.
+
----------------
Typo "headrer"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82490