ENG-1471, code is based on this diff
Changes processMessageStoreOperationsSync and processThreadStoreOperationsSync to throw JSErrors instead of returning a boolean. Corresponding javascript code was changed from checking return value to a try/catch.
Details
Details
Test on both Android and iOS.
- Place temporary throw std::system_error(ECANCELED, std::generic_category(), "error"); outside the for loop in processMessageStoreOperationsSync and build the app.
- Try and call the method with an empty array somewhere from javascript.
- The app shouldn't crash and should display that there was a JSError.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- arcpatch-D5201
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
LGTM, but don't consider myself familiar with C++ best practices, so I'll defer to someone else for marking this as accepted.
Comment Actions
This is definitely my mistake that I haven't landed https://phab.comm.dev/D4976 already. This differential contains lots of repeated code and https://phab.comm.dev/D4976 contains remeady to reduce boiler plate. I will land D4976 ASAP on Monday 26 and let you know to to update this differential.
Comment Actions
Update, removed some duplicate code.
I had to use if constexpr to differentiate between void and non-void instantiations of runSyncOrThrowJSError, because promise<void>::set_value takes no arguments so we can't call set_value(task()) (even if task returns void).
There's a warning while building android version:
warning: constexpr if is a C++17 extension [-Wc++17-extensions]
but there are already similar warnings about other code in this file:
warning: decomposition declarations are a C++17 extension [-Wc++17-extensions]
for (const auto &[message, media] : messagesVector) {and it compiles on both android and ios.
There are ways to avoid using c++17 features but they would be much uglier.
Comment Actions
warning: constexpr if is a C++17 extension [-Wc++17-extensions]
Do we set this flag? If we're successfully compiling C++17, we don't need it. It might be a good idea though to test it by building and running a production version.
One thing that is concerning is that we have set(CMAKE_CXX_STANDARD 14) in native/android/app/CMakeLists.txt so we definitely should clarify if we can use C++17 code.
We also have https://linear.app/comm/issue/ENG-637/use-c17-on-native-for-shared-mutex-support-in-clientgetreadreactor task, which suggests that we don't have full C++17 support.
| native/redux/persist.js | ||
|---|---|---|
| 365–370 ↗ | (On Diff #17108) | These calls might fail... catching exceptions and bumping version don't sounds like a good idea. |
Comment Actions
According to this stackoverflow comment (which isn't about if constexpr but still applies) compilers can backport useful features from newer versions of the standard to the previous versions if they are purely additive and wouldn't change the meaning of the code. This is probably what's happening here. There's a Wno-C++17-extensions flag if we want to disable this behavior (although we depend on c++17 in a few other places, as noted in the previous comment).
| native/redux/persist.js | ||
|---|---|---|
| 365–370 ↗ | (On Diff #17108) | The logic here doesn't change (except for the additional console.log). The previous version of the processMessageStoreOperationsSync returned false if there were any exceptions. |
Comment Actions
Talked about the if-constexpr issue with @michal in our 1:1 today. My suggestion is that we can proceed with using it, since the danger here is pretty low – worst case we have some build failures, and we revert the commit.
It would be good to test it more though. We should test:
- iOS release build
- iOS debug build
- Android release build
- Android debug build with getBuildTypeABIs here modified to return allAbis
| native/redux/persist.js | ||
|---|---|---|
| 365–370 ↗ | (On Diff #17108) | My comment doesn't make too much sense - sorry for the confusion. We're setting the cookie to null and that should be enough to solve the issue. |