Page MenuHomePhabricator
Differential D5647

Implement DraftStoreOperations C++ utility
ClosedPublic
Actions

Authored by marcin on Nov 16 2022, 5:11 AM.
Tags
None
Referenced Files
F3382005: D5647.id18688.diff
Thu, Nov 28, 7:49 AM
F3381709: D5647.diff
Thu, Nov 28, 5:58 AM
Unknown Object (File)
Wed, Nov 27, 3:30 AM
Unknown Object (File)
Tue, Nov 26, 5:34 AM
Unknown Object (File)
Tue, Nov 26, 5:34 AM
Unknown Object (File)
Tue, Nov 19, 2:22 PM
Unknown Object (File)
Tue, Nov 19, 2:22 PM
Unknown Object (File)
Tue, Nov 19, 2:22 PM
View All Files
Subscribers
• abosh
ashoat

Details

Reviewers
tomek
atul
• jon
Commits
rCOMMee8553ccdb3d: Implement DraftStoreOperations C++ utility
Summary

This differential implements DraftStoreOperations C++ utility to convert drafts related redux actions payloads to DatabaseQueryExecutor calls.

Test Plan

This differential only introduces C++ header file, so it is not possible to create test plan.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-2137
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Nov 16 2022, 5:11 AM
Herald added a reviewer: • jon. · View Herald TranscriptNov 16 2022, 5:11 AM
Herald added subscribers: • abosh, ashoat. · View Herald Transcript
marcin added a parent revision: D5646: Implement draft reducer and use it in master reducer.Nov 16 2022, 5:11 AM
marcin added a child revision: D5648: Implement draft store operations processing in CommCoreModule.Nov 16 2022, 5:17 AM
Harbormaster completed remote builds in B13473: Diff 18472.Nov 16 2022, 5:21 AM
marcin requested review of this revision.Nov 16 2022, 5:21 AM
marcin planned changes to this revision.Nov 17 2022, 2:57 AM
marcin requested review of this revision.Nov 17 2022, 8:23 AM
atul accepted this revision.Nov 17 2022, 12:43 PM

Looks clean

This revision is now accepted and ready to land.Nov 17 2022, 12:43 PM
tomek accepted this revision.Nov 18 2022, 3:42 AM
tomek added inline comments.
native/cpp/CommonCpp/NativeModules/DraftStoreOperations.h
7

Why do we need a virtual destructor?

ashoat added inline comments.Nov 18 2022, 4:55 AM
native/cpp/CommonCpp/NativeModules/DraftStoreOperations.h
7

I think we should always have a virtual destructor for anything that gets subtyped. Note that we are currently seeing compiler warnings in the native build because MessageSpec does not specify a virtual destructor

marcin added inline comments.Nov 18 2022, 5:15 AM
native/cpp/CommonCpp/NativeModules/DraftStoreOperations.h
7

In CommCoreModule we will (introduced in further diffs in this stack) have function that returns object of type std::vector<std::unique_ptr<DraftStoreOperationBase>>. When it gets destroyed, destructor of each element in this vector will be called and without destructor being virtual only memory allocated for DraftStoreOperationBase would be deleted.

Note that we are currently seeing compiler warnings in the native build because MessageSpec does not specify a virtual destructor

Without any doubt it was a mistake not to introduce it. I will create Linear task to introduce virtual destructors for any C++ class in the codebase that is meant to be derived from.

marcin updated this revision to Diff 18644.Nov 21 2022, 3:34 AM

Implement remove all drafts operation in C++

Harbormaster completed remote builds in B13614: Diff 18644.Nov 21 2022, 3:47 AM
ashoat added a comment.Nov 21 2022, 4:12 AM

I will create Linear task to introduce virtual destructors for any C++ class in the codebase that is meant to be derived from.

Thank you @marcin!! Please link before landing :)

marcin updated this revision to Diff 18688.Nov 21 2022, 9:40 AM

Rebase master

Harbormaster completed remote builds in B13641: Diff 18688.Nov 21 2022, 9:54 AM
marcin updated this revision to Diff 18757.Nov 23 2022, 4:54 AM

Rebasew before landing

Harbormaster completed remote builds in B13691: Diff 18757.Nov 23 2022, 5:06 AM
Closed by commit rCOMMee8553ccdb3d: Implement DraftStoreOperations C++ utility (authored by marcin). · Explain WhyNov 23 2022, 5:51 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMee8553ccdb3d: Implement DraftStoreOperations C++ utility.
marcin added a comment.Nov 23 2022, 5:53 AM

Issue to track virtual destructors: https://linear.app/comm/issue/ENG-2329/make-sure-all-virtual-classes-in-c-code-base-have-virtual-destructor

Revision Contents
Changeset List

PathSize
native/
cpp/
CommonCpp/
NativeModules/
40 lines

Diff 18472

View Options

native/cpp/CommonCpp/NativeModules/DraftStoreOperations.h

Loading...