[ios] Remove the dependency to base/ files in closure.h [chromium/src : main]
Asami Doi (Gerrit)
Asami Doi added 1 comment![]( "Open in Gerrit")
int ref_count_ = 1;We need a manual counting mechanism for `ClosureProvider` because we can't inherit `base::RefCounted<>`. It would be nice if we have a better way.
Related details
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad added 2 comments![]( "Open in Gerrit")
This is interesting, but I think it's too much red tape. We should see what it looks like if we used `std::function`. If that is nice and clean, then we could use simpler shims that wrap base::RepeatingCallback objects inside std::function objects. `std::function` is banned in Chromium (see: https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++-features.md#std_function-banned ). However, I think that a base::RepeatingCallback wrapped in an std::function for the purpose of binding with swift may be an acceptable exception to the ban. We should try that to see what it looks like. If it's way better, then we should ask c...@chromium.org for further guidance, and request possible amendments to the style guide.
void Release_ClosureProvider(ClosureProvider* provider) {This is not thread safe. Have you tried using std::shared_ptr?
Related details
- Asami Doi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad added 1 comment![]( "Open in Gerrit")
void Release_ClosureProvider(ClosureProvider* provider) {This is not thread safe. Have you tried using std::shared_ptr?
Perhaps it's okay to not be thread safe if this pattern is guaranteed to yield a non-Sendable closure on the swift side.
Related details
- Asami Doi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Asami Doi (Gerrit)
Asami Doi added 1 comment![]( "Open in Gerrit")
Asami DoiThis is interesting, but I think it's too much red tape. We should see what it looks like if we used `std::function`. If that is nice and clean, then we could use simpler shims that wrap base::RepeatingCallback objects inside std::function objects. `std::function` is banned in Chromium (see: https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++-features.md#std_function-banned ). However, I think that a base::RepeatingCallback wrapped in an std::function for the purpose of binding with swift may be an acceptable exception to the ban. We should try that to see what it looks like. If it's way better, then we should ask c...@chromium.org for further guidance, and request possible amendments to the style guide.
I have tried to use `std::function`. The diff is here:
https://chromium-review.googlesource.com/c/chromium/src/+/7572162/5..6
That allows us to call directly a closure by `closure()` in Swift the code becomes simpler.
I'll ask c...@chromium.org if we can accept the exception if the latest CL looks better.
Related details
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad added 3 comments![]( "Open in Gerrit")
int ref_count_ = 1;We need a manual counting mechanism for `ClosureProvider` because we can't inherit `base::RefCounted<>`. It would be nice if we have a better way.
There's probably a better way, but it's outside the scope of the CL to investigate that.
std::function<void()> ClosureProvider::GetRepeatingClosure() {This is great, but I feel the wrapping should be defined in a re-usable generic way. You could use variadic templates for this
Something like this (I did not try, may have gotten syntax details wrong):
```
namespace swift_helpers {
template<ReturnVal, typename... Args>
std::function<ReturnVal(Args)> AsStdFunction(base::RepeatingCallback<ReturnVal(Args)> cb) {
return [cb](Args... args) { cb.run(args...); }
}
template<ReturnVal, typename... Args>
std::function<ReturnVal(Args)> AsStdFunction(base::OnceCallback<ReturnVal(Args)> cb) {
auto shared_cb = std::make_shared<base::OnceClosure>(std::move(cb));
return [shared_cb](Args... args) mutable {
if (!shared_cb->is_null()) {
std::move(*shared_cb).Run(args...);
}
}
}
}
```
}Nice workaround!
Have you tried finding a way to export a std::function as a move-only type?
On the swift side, closest the equivalent of of R-value references is 'consuming' arguments. There may be a way of leveraging that to ensure that once callbacks can only be invoked once from the swift side.
Alternately, if we can't provide compile-time guarantees that the swift code can only call the callback once (or if we want to solve that problem later), then there should at least be a run-time check. There could be a `CHECK` statement, or perhaps an `else` clause that calls `base::debug::DumpWuithoutCrashing` when `shared_cb` is null.
Related details
- Asami Doi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Asami Doi (Gerrit)
Asami Doi added 5 comments![]( "Open in Gerrit")
int ref_count_ = 1;Justin NovosadWe need a manual counting mechanism for `ClosureProvider` because we can't inherit `base::RefCounted<>`. It would be nice if we have a better way.
There's probably a better way, but it's outside the scope of the CL to investigate that.
I've wrapped ref_count_ with std::atomic. The unsafe thread issue is solved. I'll improve this part in another CL.
std::function<void()> ClosureProvider::GetRepeatingClosure() {This is great, but I feel the wrapping should be defined in a re-usable generic way. You could use variadic templates for this
Something like this (I did not try, may have gotten syntax details wrong):
```
namespace swift_helpers {template<ReturnVal, typename... Args>
std::function<ReturnVal(Args)> AsStdFunction(base::RepeatingCallback<ReturnVal(Args)> cb) {
return [cb](Args... args) { cb.run(args...); }
}template<ReturnVal, typename... Args>
std::function<ReturnVal(Args)> AsStdFunction(base::OnceCallback<ReturnVal(Args)> cb) {
auto shared_cb = std::make_shared<base::OnceClosure>(std::move(cb));
return [shared_cb](Args... args) mutable {
if (!shared_cb->is_null()) {
std::move(*shared_cb).Run(args...);
}
}
}}
```
Thank you for the suggestion. I've implemented the generic ToStdFunction templates using variadic arguments as suggested. This handles both RepeatingCallback and OnceCallback (with the shared_ptr workaround and CHECK safety).
Nice workaround!
Have you tried finding a way to export a std::function as a move-only type?
On the swift side, closest the equivalent of of R-value references is 'consuming' arguments. There may be a way of leveraging that to ensure that once callbacks can only be invoked once from the swift side.
Alternately, if we can't provide compile-time guarantees that the swift code can only call the callback once (or if we want to solve that problem later), then there should at least be a run-time check. There could be a `CHECK` statement, or perhaps an `else` clause that calls `base::debug::DumpWuithoutCrashing` when `shared_cb` is null.
Thanks for the suggestion. I don't find the way to export std::function as a move-only type. I have tried to mark it with the annotation (`__attribute__((swift_attr("~Copyable")))`) but it doesn't work as intended.
I've added a CHECK(!shared_cb->is_null()) inside the lambda. This ensures a crash if the Swift code attempts to invoke the OnceClosure more than once.
void Release_ClosureProvider(ClosureProvider* provider) {Justin NovosadThis is not thread safe. Have you tried using std::shared_ptr?
Perhaps it's okay to not be thread safe if this pattern is guaranteed to yield a non-Sendable closure on the swift side.
I have tried to return std::shared_ptr<ClosureProvider> from MakeForSwift() but it didn't work well.
Instead, I wrap ref_count_ with std::atomic.
Related details
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
std::function<void()> GetRepeatingClosure();`std::function<...>` is banned in Chromium code.
Related details
- Asami Doi
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Asami Doi (Gerrit)
Asami Doi added 1 comment![]( "Open in Gerrit")
std::function<void()> GetRepeatingClosure();`std::function<...>` is banned in Chromium code.
Yes, so I'll ask c...@chromium.org if we can accept using std::function for the C++/Swift interop case as an exception once the CL becomes stable.
Related details
- Justin Novosad
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad added 2 comments![]( "Open in Gerrit")
template <typename ReturnVal, typename... Args>
std::function<ReturnVal(Args...)> ToStdFunction(
base::RepeatingCallback<ReturnVal(Args...)> cb) {
return [cb = std::move(cb)](Args... args) -> ReturnVal {
return cb.Run(std::forward<Args>(args)...);
};
}
template <typename ReturnVal, typename... Args>
std::function<ReturnVal(Args...)> ToStdFunction(
base::OnceCallback<ReturnVal(Args...)> cb) {
using CallbackType = base::OnceCallback<ReturnVal(Args...)>;
// std::function requires its target to be copyable.
// By wrapping the base::OnceCallback in a std::shared_ptr, the lambda
// captures the pointer instead of the callback itself.
auto shared_cb = std::make_shared<CallbackType>(std::move(cb));
// The mutable keyword allows to move a value.
return [shared_cb](Args... args) mutable -> ReturnVal {
CHECK(shared_cb && !shared_cb->is_null())
<< "OnceCallback invoked more than once from Swift.";
return std::move(*shared_cb).Run(std::forward<Args>(args)...);
};
}Excellent!
Now can we move these to a header somewhere since this is re-usable code that we'll eventually want to use in the product (not just in tests).
Asami DoiNice workaround!
Have you tried finding a way to export a std::function as a move-only type?
On the swift side, closest the equivalent of of R-value references is 'consuming' arguments. There may be a way of leveraging that to ensure that once callbacks can only be invoked once from the swift side.
Alternately, if we can't provide compile-time guarantees that the swift code can only call the callback once (or if we want to solve that problem later), then there should at least be a run-time check. There could be a `CHECK` statement, or perhaps an `else` clause that calls `base::debug::DumpWuithoutCrashing` when `shared_cb` is null.
Thanks for the suggestion. I don't find the way to export std::function as a move-only type. I have tried to mark it with the annotation (`__attribute__((swift_attr("~Copyable")))`) but it doesn't work as intended.
I've added a CHECK(!shared_cb->is_null()) inside the lambda. This ensures a crash if the Swift code attempts to invoke the OnceClosure more than once.
I think this could be solved by using a type with a deleted copy constructor, but I'm not sure what exact incantaion will work well with interop. We can revisit this in a follow-up CL.
Related details
- Asami Doi
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad added 1 comment![]( "Open in Gerrit")
std::function<void()> GetRepeatingClosure();Asami Doi`std::function<...>` is banned in Chromium code.
Yes, so I'll ask c...@chromium.org if we can accept using std::function for the C++/Swift interop case as an exception once the CL becomes stable.
I think we've converged enough at this point to loop in C++ style owners. I think the main point we need to make here is that we're not using std::function on its own. We're using it as a wrapper to transport base::{Repeating|Once}Callback objects across the C++-to-swift boundary. This way, we continue to benefit from the safety features built into the base callback types.
Asami Doi (Gerrit)
Asami Doi added 2 comments![]( "Open in Gerrit")
std::function<void()> GetRepeatingClosure();Asami Doi`std::function<...>` is banned in Chromium code.
Justin NovosadYes, so I'll ask c...@chromium.org if we can accept using std::function for the C++/Swift interop case as an exception once the CL becomes stable.
I think we've converged enough at this point to loop in C++ style owners. I think the main point we need to make here is that we're not using std::function on its own. We're using it as a wrapper to transport base::{Repeating|Once}Callback objects across the C++-to-swift boundary. This way, we continue to benefit from the safety features built into the base callback types.
I have sent an email to c...@chromium.org with you in CC. Let's wait for their feedback.
template <typename ReturnVal, typename... Args>
std::function<ReturnVal(Args...)> ToStdFunction(
base::RepeatingCallback<ReturnVal(Args...)> cb) {
return [cb = std::move(cb)](Args... args) -> ReturnVal {
return cb.Run(std::forward<Args>(args)...);
};
}
template <typename ReturnVal, typename... Args>
std::function<ReturnVal(Args...)> ToStdFunction(
base::OnceCallback<ReturnVal(Args...)> cb) {
using CallbackType = base::OnceCallback<ReturnVal(Args...)>;
// std::function requires its target to be copyable.
// By wrapping the base::OnceCallback in a std::shared_ptr, the lambda
// captures the pointer instead of the callback itself.
auto shared_cb = std::make_shared<CallbackType>(std::move(cb));
// The mutable keyword allows to move a value.
return [shared_cb](Args... args) mutable -> ReturnVal {
CHECK(shared_cb && !shared_cb->is_null())
<< "OnceCallback invoked more than once from Swift.";
return std::move(*shared_cb).Run(std::forward<Args>(args)...);
};
}Excellent!
Now can we move these to a header somewhere since this is re-usable code that we'll eventually want to use in the product (not just in tests).
- Justin Novosad
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Asami Doi (Gerrit)
Asami Doi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Asami Doi
| Commit-Queue | +1 |
2 comments
std::function<void()> GetRepeatingClosure();Asami Doi`std::function<...>` is banned in Chromium code.
Justin NovosadYes, so I'll ask c...@chromium.org if we can accept using std::function for the C++/Swift interop case as an exception once the CL becomes stable.
Asami DoiI think we've converged enough at this point to loop in C++ style owners. I think the main point we need to make here is that we're not using std::function on its own. We're using it as a wrapper to transport base::{Repeating|Once}Callback objects across the C++-to-swift boundary. This way, we continue to benefit from the safety features built into the base callback types.
I have sent an email to c...@chromium.org with you in CC. Let's wait for their feedback.
The usage of std::function can be accepted in this limited scope. https://groups.google.com/a/chromium.org/g/cxx/c/dj6q-e4wXPc
I have added the exception to PRESUBMIT.py.
Related details
- Justin Novosad
- Nico Weber
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad voted and added 2 comments![]( "Open in Gerrit")
Votes added by Justin Novosad
| Code-Review | +1 |
2 comments
template <typename ReturnVal, typename... Args>Add comments that explain how and when to use these helpers.
closure()The fact that we can get rid of RunCxxClosure is fantastic.
Related details
- Asami Doi
- Nico Weber
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nico Weber
| Code-Review | +1 |
3 comments
lg once junov is happy
template <typename ReturnVal, typename... Args>Add comments that explain how and when to use these helpers.
+1
namespace swift_helpers {Probably should be base::swift_helpers if you want to put this in base (?)
Related details
- Asami Doi
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Enrique Cab (Gerrit)
Enrique Cab added 2 comments![]( "Open in Gerrit")
Nico WeberAdd comments that explain how and when to use these helpers.
+1
Done
The fact that we can get rid of RunCxxClosure is fantastic.
Marked as resolved.
Related details
- Asami Doi
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Asami Doi (Gerrit)
Asami Doi added 3 comments![]( "Open in Gerrit")
template <typename ReturnVal, typename... Args>Nico WeberAdd comments that explain how and when to use these helpers.
+1
Done
namespace swift_helpers {Probably should be base::swift_helpers if you want to put this in base (?)
I have added `namespace base`. We can move this file to a different directory in a follow-up CL if we find a better place.
closure()The fact that we can get rid of RunCxxClosure is fantastic.
- Enrique Cab
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |