Allow trivially move constructible types to be relocatable #1035
Conversation
This file contains 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
There was a problem hiding this comment.
@yfeldblum has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
yfeldblum
requested changes
Feb 27, 2019
JoeLoser
force-pushed
the
is_relocatable
branch
2 times, most recently
from
77fd7f7 to
fc0402f
Compare
yfeldblum
requested changes
Feb 27, 2019
Summary: - If a type is not marked explicitly with `IsRelocatable` as `std::true_type`, we infer its relocability only based on the property of whether the type is trivially copyable. - Extend the default relocability to also be true for trivially move constructible types.
JoeLoser
force-pushed
the
is_relocatable
branch
from
fc0402f to
029f843
Compare
There was a problem hiding this comment.
@yfeldblum has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
We had to revert this since some places with gcc49 broke. |
sandraiser
pushed a commit
to sandraiser/folly
that referenced
this pull request
Mar 4, 2019
…1035) Summary: - If a type is not marked explicitly with `IsRelocatable` as `std::true_type`, we infer its relocability only based on the property of whether the type is trivially copyable. - Extend the default relocability to also be true for trivially move constructible types. Pull Request resolved: facebook#1035 Reviewed By: Orvid Differential Revision: D14240127 Pulled By: yfeldblum fbshipit-source-id: 1e15d312d1a8340417bba2beb1db30ce4c543b26
|
When we get everyone off of GCC 4.9, think we can try landing this again? |
|
That's the plan! |
|
If you think the recent change to keep GCC5 as min compiler, feel free to try landing this again. 🤞 |
|
Per the other PR, I think the issue remains with clang using old libstdc++ in some Android NDKs. |
Quuxplusone
added a commit
to Quuxplusone/folly
that referenced
this pull request
Dec 31, 2023
See 93b0f54 (introducing the TODO comment, Dec 2012-12-05); bb19b30 (implementing it in code, 2019-02-27, GitHub PR facebook#1035); 3f7a734 (reverting it back to a comment, 2019-02-28). My understanding is that the revert was for toolchain reasons, not semantic reasons. My argument below is for semantic reasons. As I understand the comment, it's saying that we can productively assume that any type that's trivially move-assignable is also trivially relocatable; thus we could change : std::conditional< sizeof(T) && is_detected_v<traits_detail::detect_IsRelocatable, T>, traits_detail::has_true_IsRelocatable<T>, - is_trivially_copyable<T>>::type {}; + std::is_trivially_move_constructible<T>>::type {}; This makes a certain amount of sense: "move-assign" means "discard the LHS's old value, take on the RHS's value, don't change the number of extant objects," which is superficially similar to relocation's meaning "take on the RHS's value, discard the RHS's value, don't change the number of extant objects." - Pro: I can't think of a _non-contrived_ example of a type which is trivially move-assignable but not trivially relocatable. That is, the proposed change would be mostly harmless. However: - Con: The most common trivially-relocatable types (vector, unique_ptr) are not trivially move-constructible, so this would be a no-op on all types except very contrived ones. - In fact, I would go so far as to say that any type which is trivially move-constructible yet NOT trivially copyable is probably a bug! - Con: An RAII type such as `struct S { S(); ~S(); }` is trivially move-constructible, but not trivially copyable because it's not trivially destructible; and without further annotation, we should NOT assume that `S` is trivially relocatable (because that non-trivial destructor might be significant to the user). - Suppose `S` were (Platonically) trivially relocatable after all. Then adding `S& operator=(S&&) = delete` to it shouldn't change the fact that it's trivially relocatable (e.g. when a `vector<S>` needs to reallocate). Therefore anything based on `is_*_assignable` is the wrong tool for the job; it'll report `false` for some types that we could in fact trivially relocate. (`is_trivially_copyable` and the proposed `is_trivially_relocatable` will still report `true` for types with deleted special members.) Conclusion: The current code is already at a (local) optimum; replacing `is_trivially_copyable` with anything involving `is_trivially_move_assignable` would be a regression. Replacing it with `__is_trivially_relocatable(T)` on compilers that support it (currently just Clang) could be a good idea. For now, simply removing the TODO seems like a step in the right direction.
Quuxplusone
added a commit
to Quuxplusone/folly
that referenced
this pull request
Dec 31, 2023
See 93b0f54 (introducing the TODO comment, Dec 2012-12-05); bb19b30 (implementing it in code, 2019-02-27, GitHub PR facebook#1035); 3f7a734 (reverting it back to a comment, 2019-02-28). My understanding is that the revert was for toolchain reasons, not semantic reasons. My argument below is for semantic reasons. As I understand the comment, it's saying that we can productively assume that any type that's trivially move-constructible is also trivially relocatable; thus we could change : std::conditional< sizeof(T) && is_detected_v<traits_detail::detect_IsRelocatable, T>, traits_detail::has_true_IsRelocatable<T>, - is_trivially_copyable<T>>::type {}; + std::is_trivially_move_constructible<T>>::type {}; - Pro: I can't think of a _non-contrived_ example of a type which is trivially move-constructible but not trivially relocatable. That is, the proposed change would be mostly harmless. However: - Con: The most common trivially-relocatable types (vector, unique_ptr) are not trivially move-constructible, so this would be a no-op on all types except very contrived ones. - In fact, I would go so far as to say that any type which is trivially move-constructible yet NOT trivially copyable is probably a bug! - Con: An RAII type such as `struct S { S(); ~S(); }` is trivially move-constructible, but not trivially copyable because it's not trivially destructible; and without further annotation, we should NOT assume that `S` is trivially relocatable (because that non-trivial destructor might be significant to the user). - Suppose `S` were (Platonically) trivially relocatable after all. Then adding `S(S&&) = delete` to it shouldn't change the fact that it's trivially relocatable (e.g. in `vector<S>::erase`, which doesn't require move-construction, just move-assignment and destruction). Therefore anything based on `is_*_constructible` seems like the wrong tool for the job; it'll report `false` for some types that we could in fact trivially relocate. (`is_trivially_copyable` and the proposed `is_trivially_relocatable` will still report `true` for types with deleted special members.) Conclusion: The current code is already at a (local) optimum; replacing `is_trivially_copyable` with anything involving `is_trivially_move_constructible` would be a regression. Replacing it with `__is_trivially_relocatable(T)` on compilers that support it (currently just Clang) could be a good idea. For now, simply removing the TODO seems like a step in the right direction.
Quuxplusone
added a commit
to Quuxplusone/folly
that referenced
this pull request
Dec 31, 2023
See 93b0f54 (introducing the TODO comment, 2012-12-05); bb19b30 (implementing it in code, 2019-02-27, GitHub PR facebook#1035); 3f7a734 (reverting it back to a comment, 2019-02-28). My understanding is that the revert was for toolchain reasons, not semantic reasons. My argument below is for semantic reasons. As I understand the comment, it's saying that we can productively assume that any type that's trivially move-constructible is also trivially relocatable; thus we could change : std::conditional< sizeof(T) && is_detected_v<traits_detail::detect_IsRelocatable, T>, traits_detail::has_true_IsRelocatable<T>, - is_trivially_copyable<T>>::type {}; + std::is_trivially_move_constructible<T>>::type {}; - Pro: I can't think of a _non-contrived_ example of a type which is trivially move-constructible but not trivially relocatable. That is, the proposed change would be mostly harmless. However: - Con: The most common trivially-relocatable types (vector, unique_ptr) are not trivially move-constructible, so this would be a no-op on all types except very contrived ones. - In fact, I would go so far as to say that any type which is trivially move-constructible yet NOT trivially copyable is probably a bug! - Con: An RAII type such as `struct S { S(); ~S(); }` is trivially move-constructible, but not trivially copyable because it's not trivially destructible; and without further annotation, we should NOT assume that `S` is trivially relocatable (because that non-trivial destructor might be significant to the user). - Suppose `S` were (Platonically) trivially relocatable after all. Then adding `S(S&&) = delete` to it shouldn't change the fact that it's trivially relocatable (e.g. in `vector<S>::erase`, which doesn't require move-construction, just move-assignment and destruction). Therefore anything based on `is_*_constructible` seems like the wrong tool for the job; it'll report `false` for some types that we could in fact trivially relocate. (`is_trivially_copyable` and the proposed `is_trivially_relocatable` will still report `true` for types with deleted special members.) Conclusion: The current code is already at a (local) optimum; replacing `is_trivially_copyable` with anything involving `is_trivially_move_constructible` would be a regression. Replacing it with `__is_trivially_relocatable(T)` on compilers that support it (currently just Clang) could be a good idea. For now, simply removing the TODO seems like a step in the right direction.
facebook-github-bot
pushed a commit
that referenced
this pull request
Jan 23, 2024
Summary: (attn JoeLoser, mengz0, yfeldblum) See 93b0f54 (introducing the TODO comment, 2012-12-05); bb19b30 (implementing it in code, 2019-02-27, GitHub PR #1035); 3f7a734 (reverting it back to a comment, 2019-02-28). My understanding is that the revert was for toolchain reasons, not semantic reasons. My argument below is for semantic reasons. As I understand the comment, it's saying that we can productively assume that any type that's trivially move-constructible is also trivially relocatable; thus we could change : std::conditional< sizeof(T) && is_detected_v<traits_detail::detect_IsRelocatable, T>, traits_detail::has_true_IsRelocatable<T>, - is_trivially_copyable<T>>::type {}; + std::is_trivially_move_constructible<T>>::type {}; - Pro: I can't think of a _non-contrived_ example of a type which is trivially move-constructible but not trivially relocatable. That is, the proposed change would be mostly harmless. However: - Con: The most common trivially-relocatable types (vector, unique_ptr) are not trivially move-constructible, so this would be a no-op on all types except very contrived ones. - In fact, I would go so far as to say that any type which is trivially move-constructible yet NOT trivially copyable is probably a bug! - Con: An RAII type such as `struct S { S(); ~S(); }` is trivially move-constructible, but not trivially copyable because it's not trivially destructible; and without further annotation, we should NOT assume that `S` is trivially relocatable (because that non-trivial destructor might be significant to the user). - Suppose `S` were (Platonically) trivially relocatable after all. Then adding `S(S&&) = delete` to it shouldn't change the fact that it's trivially relocatable (e.g. in `vector<S>::erase`, which doesn't require move-construction, just move-assignment and destruction). Therefore anything based on `is_*_constructible` seems like the wrong tool for the job; it'll report `false` for some types that we could in fact trivially relocate. (`is_trivially_copyable` and the proposed `is_trivially_relocatable` will still report `true` for types with deleted special members.) Conclusion: The current code is already at a (local) optimum; replacing `is_trivially_copyable` with anything involving `is_trivially_move_constructible` would be a regression. Replacing it with `__is_trivially_relocatable(T)` on compilers that support it (currently just Clang) could be a good idea. For now, simply removing the TODO seems like a step in the right direction. Pull Request resolved: #2116 Reviewed By: Gownta Differential Revision: D52975801 Pulled By: Orvid fbshipit-source-id: a78ea99659574c56a3987631b90ecb917e5cb672
facebook-github-bot
pushed a commit
to facebook/hhvm
that referenced
this pull request
Jan 23, 2024
Summary: (attn JoeLoser, mengz0, yfeldblum) See 93b0f543f2 (introducing the TODO comment, 2012-12-05); bb19b3016d (implementing it in code, 2019-02-27, GitHub PR facebook/folly#1035); 3f7a734cee (reverting it back to a comment, 2019-02-28). My understanding is that the revert was for toolchain reasons, not semantic reasons. My argument below is for semantic reasons. As I understand the comment, it's saying that we can productively assume that any type that's trivially move-constructible is also trivially relocatable; thus we could change : std::conditional< sizeof(T) && is_detected_v<traits_detail::detect_IsRelocatable, T>, traits_detail::has_true_IsRelocatable<T>, - is_trivially_copyable<T>>::type {}; + std::is_trivially_move_constructible<T>>::type {}; - Pro: I can't think of a _non-contrived_ example of a type which is trivially move-constructible but not trivially relocatable. That is, the proposed change would be mostly harmless. However: - Con: The most common trivially-relocatable types (vector, unique_ptr) are not trivially move-constructible, so this would be a no-op on all types except very contrived ones. - In fact, I would go so far as to say that any type which is trivially move-constructible yet NOT trivially copyable is probably a bug! - Con: An RAII type such as `struct S { S(); ~S(); }` is trivially move-constructible, but not trivially copyable because it's not trivially destructible; and without further annotation, we should NOT assume that `S` is trivially relocatable (because that non-trivial destructor might be significant to the user). - Suppose `S` were (Platonically) trivially relocatable after all. Then adding `S(S&&) = delete` to it shouldn't change the fact that it's trivially relocatable (e.g. in `vector<S>::erase`, which doesn't require move-construction, just move-assignment and destruction). Therefore anything based on `is_*_constructible` seems like the wrong tool for the job; it'll report `false` for some types that we could in fact trivially relocate. (`is_trivially_copyable` and the proposed `is_trivially_relocatable` will still report `true` for types with deleted special members.) Conclusion: The current code is already at a (local) optimum; replacing `is_trivially_copyable` with anything involving `is_trivially_move_constructible` would be a regression. Replacing it with `__is_trivially_relocatable(T)` on compilers that support it (currently just Clang) could be a good idea. For now, simply removing the TODO seems like a step in the right direction. X-link: facebook/folly#2116 Reviewed By: Gownta Differential Revision: D52975801 Pulled By: Orvid fbshipit-source-id: a78ea99659574c56a3987631b90ecb917e5cb672
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.
Summary:
IsRelocatableas
std::true_type, we infer its relocability only based on theproperty of whether the type is trivially copyable.
constructible types.