Page MenuHomePhabricator

[CommCoreModule] introduce BaseDataStore template class
ClosedPublic

Authored by kamil on Jul 24 2023, 5:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jun 28, 3:17 AM
Unknown Object (File)
Fri, Jun 28, 12:46 AM
Unknown Object (File)
Tue, Jun 25, 3:51 AM
Unknown Object (File)
Mon, Jun 24, 11:13 AM
Unknown Object (File)
Sat, Jun 22, 4:09 PM
Unknown Object (File)
Wed, Jun 19, 12:08 PM
Unknown Object (File)
Wed, Jun 19, 12:08 PM
Unknown Object (File)
Wed, Jun 19, 12:08 PM
Subscribers

Details

Summary

Problems I want to solve:

  1. Code duplication - this part of code is duplicated at least 5 times (and will grow).
  1. Lack of separation - a lot of not related code like parsing is here, this can be grouped logically.

How it works:

  1. This template class provides two generic functions which will be reused (processStoreOperations, processStoreOperationsSync) - this solves problem 1.
  2. Has two pure virtual methods which need to be overridden (parseDBDataStore, createOperations), those methods are specialized for each data type (Messages, Threads, etc.) - this solves problem 2.

Why .h file?
When a template is used in C++, the compiler needs access to the entire template definition (not just a declaration) because it needs to generate the code for the specific instantiation at the compile time. (source).

Why not static methods when the class does not have a state?
I wanted to ensure that each child class (which will inherit from BaseDataStore) will override methods for parsing and creating ops. This can not be achieved easily with static methods - but I am open to modifying if someone has valid suggestions.

For more details check the next diff in the stack.

Depends on D8601

Test Plan

Build iOS and Android (rest is tested in detailed implementation in next diffs).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil edited the summary of this revision. (Show Details)
kamil published this revision for review.Jul 24 2023, 6:20 AM
atul added a subscriber: marcin.

This looks correct to me. However, my C++ is rusty—particularly w/ templates—so it'd be good to get someone else to sign off before landing (@marcin or @tomek?)

This revision is now accepted and ready to land.Jul 24 2023, 11:08 PM
kamil added a reviewer: marcin. kamil added 3 blocking reviewer(s): tomek, michal, jon.Jul 25 2023, 1:51 AM

Thanks @atul!

This is a serious diff - based on assumptions here rest of the code around will be built and I would love to have a detailed review of my approach, so adding as blocking:
@michal - has good knowledge of C++ templates
@tomek - has knowledge about CommCoreModule
@jon - to check the CMake (I am not sure about correctness)

This revision now requires review to proceed.Jul 25 2023, 1:51 AM

cmake looks fine to me

This revision is now accepted and ready to land.Jul 26 2023, 7:31 AM

add jsInvoker to base class, add virtual constructor