Page MenuHomePhabricator

[SQLite] implement getting `MessageToDevice` items
ClosedPublic

Authored by kamil on Feb 26 2024, 2:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 6:55 PM
Unknown Object (File)
Mon, Nov 11, 4:46 AM
Unknown Object (File)
Sun, Nov 10, 2:40 PM
Unknown Object (File)
Sat, Nov 9, 9:15 PM
Unknown Object (File)
Sat, Nov 9, 8:22 PM
Unknown Object (File)
Sat, Nov 9, 6:15 PM
Unknown Object (File)
Sat, Nov 9, 3:49 AM
Unknown Object (File)
Wed, Nov 6, 5:43 PM
Subscribers

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Mar 1 2024, 12:24 AM
marcin requested changes to this revision.Mar 1 2024, 2:22 AM
marcin added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1717–1719 ↗(On Diff #37691)

The convention is not to use sqlite3_stmt directly but to use SQLiteStatementWrapper that automatically handles preparing, finalizing and errors.

1730–1735 ↗(On Diff #37691)

This code doesn't perform necessary error handling for sqlite3_step and sqlite3_finalize but if you use SQLiteStatementWrapper it will do error handling automatically for you.

This revision now requires changes to proceed.Mar 1 2024, 2:22 AM
marcin added inline comments.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1721

Nit: Error message you provide here suggest that SQLite function that failed was sqlite3_finalize which is not necessarily the case. If one of sqlite3_step fails the SQLiteStatementWrapper destructor will throw an error with error message associated with the failed sqlite3_step. In other words the destructor of SQLiteStatementWrapper throws error with message built from the error code of the first function that failed. That said we should aim for something more general like: "Failed to get all messages to device".

This revision is now accepted and ready to land.Mar 5 2024, 1:27 AM
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
1721

make sense, thanks for suggestion