poboiko added a comment.
Nice! I like it, it's definitely much better than `Q_ASSERT_X` macros that
are just silently ignored in non-debug builds.
Have some nitpicks though:
1. Are we actually sure this is gonna fix all those crashes? Otherwise I
would suggest using CCBUG instead of BUG inside the commit message.
2. Looks like this patch is composed of two parts - introducing new logging
category and and improving error handling. It would be also nice to split those
into two separate patches.
3. For now, logging is the only way to know if there's something bad going
on. In that case I would suggest to at least increase severity of those
messages - it would increase chances users will notice it. For example, use
`qCCritical` (but this would also require additional check for
okayish/non-critical return codes, such as `MDB_NOTFOUND`)
4. There are a lot of redundant `Q_ASSERT_X` left, which could be removed. I
suggest just `grep`'ing over the code to catch'em all. I've started marking
them here, but then I gave up - too many of them.
5. There are also several unchecked return codes as well, such as inside
`*DB::size()` calls. Those can also be found by `grep`'ing over `Q_ASSERT_X`.
INLINE COMMENTS
> documentdatadb.cpp:142
> }
> Q_ASSERT_X(rc == 0, "DocumentDataDB::toTestMap", mdb_strerror(rc));
>
This is not needed now, I guess
> documentdb.cpp:144
> MDB_stat stat;
> int rc = mdb_stat(m_txn, m_dbi, &stat);
> Q_ASSERT_X(rc == 0, "DocumentDB::size", mdb_strerror(rc));
This can also return something weird.
> postingdb.cpp:132
> + while (rc == 0) {
> Q_ASSERT_X(rc == 0, "PostingDB::fetchTermsStartingWith",
> mdb_strerror(rc));
>
This assert is not necessary here then?
REPOSITORY
R293 Baloo
REVISION DETAIL
https://phabricator.kde.org/D18664
To: valeriymalov, #baloo, bruns, poboiko
Cc: ngraham, bruns, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh,
astippich, spoorun, abrahams
