Make is_complete static assertion unconditional for unique_ptr<T> bindings.
#1536
+372
−29
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
This commit adds minimal test infrastructure for inspecting the error messages reported by a C++ compiler when compiling the `.cc` file generated by `cxx_gen::generate_header_and_cc`.
Before this commit, `fn write_unique_ptr_common` in `gen/src/write.rs`
would avoid emitting the following code elements if it the `T` of
`std::unique_ptr<T>` was surely fully defined:
* `static_assert(::rust::is_complete<T>...)`
* `::rust::deleter_if<::rust::detail::is_complete<T>::value>{}(ptr)`
Otherwise, the `static_assert` wasn't emitted, and deletion would be
a simple `ptr->~unique_ptr()` call.
After this commit, the `static_assert`, and the `deleter_if`-guarded
delation are *always* used. This has the following impact:
* This simplifies the code in `gen/src/write.rs`. In particular, it
helps to refactor that code to support arbitrary `T` in
`UniquePtr<T>`, by avoiding the need to calculate `conditional_delete`
for arbirary `T`.
* This may potentially impact performance of C++ compilation runtime.
OTOH:
- The impact seems relatively small in the grand scheme of things.
- The optimization before this commit was based on a heuristic.
For example, the extra code would still be emitted for
`UniquePtr<u32>`.
anforowicz
force-pushed
the
conditional-delete-should-be-unconditional
branch
from
a513fcb to
fd4c08c
Compare
|
Force push above rebases on top of the upstream origin/master - this should make this PR mergeable again. |
|
It seems that this PR will fix #188 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PTAL?
FWIW I tried to leave a justification of small design/coding decisions here and there in code comments + in the description of the last commit.
Similarly to my last PR, this one is also secretly part of a bigger work item (*). I hope that when the bigger work item is done, then
enum UniquePtr,trait ToTypename, andtrait ToMangledcan be removed altogether fromgen/src/write.rs. But even if this doesn't materialize, the changes from this PR under review seem desirable on their own (adding test coverage for C++-side errors + simplifyingfn write_unique_ptr_commonand removing some uses/deconstructions ofUniquePtr).WDYT?
(*) "bigger work item" has evolved from trying to add support for
Vec<[u8; N]>to adding support forVec<T>whereTis not necessarily a simple identifier (e.g. forVec<Vec<T>>). I am still working at this point on the last few remaining places wherewrite.rsorexpand.rsonly work with identifier-only/named types (one of those places isconditional_delete, another isis_maybe_trivial, andfn write_unique_ptr_commonis another one).