Details

Reviewers

marcin
• przemek
michal
bartek
• jon
ashoat
tomek

Commits

rCOMM9fb642cd6962: Refactor some SQLiteQueryExecutor code

Summary

Refactor code handling Metadata table to avoid repeating code. Functions used before have been left as wrappers as suggested in comments on ENG-1979.

Test Plan

Running the ios simulator. I feel like, since there is no functional change, and the changes are pretty simple, it should be enough to check if everything compiles, but if I should test
it more thoroughly please let me know.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

inka created this revision.Oct 7 2022, 7:58 AM

Herald added a reviewer: • jon. · View Herald TranscriptOct 7 2022, 7:58 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

Adding Ashoat because the changes are related to our discussion on ENG-1979

Harbormaster completed remote builds in B12673: Diff 17417.Oct 7 2022, 8:08 AM

inka requested review of this revision.Oct 7 2022, 8:08 AM

Looks good to me!! Would be great if @marcin could take a look as well

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
881 ↗	(On Diff #17417)	Nit – I link to keep `this->` on these so it's clear the function is on the class, and not just a standalone function

Looks good!
I also favour using this-> when referencing class members.

Add this-> to class member calls

Harbormaster completed remote builds in B12682: Diff 17432.Oct 10 2022, 12:48 AM

In D5316#156915, @bartek wrote:

Looks good!
I also favour using this-> when referencing class members.

At some point we agreed to use this -> https://www.notion.so/commapp/C-coding-standards-8c9f22b4b16a41c3a868eb5f537db1de

marcin requested changes to this revision.Oct 10 2022, 3:15 AM

marcin added inline comments.

native/cpp/CommonCpp/DatabaseManagers/DatabaseQueryExecutor.h
63–65 ↗	(On Diff #17432)	Those methods should be private
63–65 ↗	(On Diff #17432)	They can take their arguments by reference.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h
63–65 ↗	(On Diff #17432)	Same.

This revision now requires changes to proceed.Oct 10 2022, 3:15 AM

Make metadata functions private. I'm not really happy with making them private this way, but I was trying to follow the convention set by the migrate function.

Harbormaster failed remote builds in B12689: Diff 17439!Oct 10 2022, 3:53 AM

Looking at signatures of other methods in DatabaseQueryExecutor I can see that they also do not take their parameters by reference. The reason is probably thread safety so we can ignore my previous comment that suggested passing arguments to setMetadata, getMetadata and clearMetadata by reference.

tomek accepted this revision.Oct 10 2022, 5:55 AM

This revision is now accepted and ready to land.Oct 10 2022, 5:55 AM

Harbormaster completed remote builds in B12689: Diff 17439.Oct 10 2022, 6:14 AM

cherry pick

Harbormaster completed remote builds in B12695: Diff 17447.Oct 10 2022, 6:52 AM

Looks good to me!

inka mentioned this in D5339: [native] Add wrapper functions for getting and setting device id in cpp.Oct 11 2022, 12:38 AM

inka added a child revision: D5339: [native] Add wrapper functions for getting and setting device id in cpp.Oct 11 2022, 12:39 AM

Closed by commit rCOMM9fb642cd6962: Refactor some SQLiteQueryExecutor code (authored by • InkaSokolowska). · Explain WhyOct 30 2022, 2:05 PM

This revision was automatically updated to reflect the committed changes.

• InkaSokolowska added a commit: rCOMM9fb642cd6962: Refactor some SQLiteQueryExecutor code.

• InkaSokolowska mentioned this in rCOMM61be49f2b738: [native] Add wrapper functions for getting and setting device id in cpp.