[webkit-reviews] review granted: [Bug 47859] sqlite: show extended error codes in error logs : [Attachment 71098] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 19 12:04:05 PDT 2010


David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Evan Martin
<evan at chromium.org>'s request for review:
Bug 47859: sqlite: show extended error codes in error logs
https://bugs.webkit.org/show_bug.cgi?id=47859

Attachment 71098: Patch
https://bugs.webkit.org/attachment.cgi?id=71098&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71098&action=review

r=me, but please address the questions below before committing.

>> WebCore/platform/sql/SQLiteDatabase.cpp:76
>> +	m_lastError = sqlite3_extended_result_codes(m_db, 1);
> 
> I'm not sure why m_lastError is a member variable.  It doesn't appear to be
used other than in the open function.
> 
> Also, it looks like we only get here when m_lastError == SQLITE_OK.  Will
sqlite3_extended_result_codes provide information when SQLITE_OK?  It seems
weird that sqlite3_extended_result_codes returns the previous error.

Making m_lastError a local variable should be a separate patch, but I agree
that it doesn't need to be a member variable.

The sqlite3_extended_result_codes() function enables extended error codes for
future database operations.

My only concern is whether enabling extended error codes has any performance
impact.  (I suspect it doesn't because the error codes are just integers.)

> WebCore/platform/sql/SQLiteStatement.cpp:108
> +	       error, m_query.ascii().data(), error,
sqlite3_errmsg(m_database.sqlite3Handle()));

Isn't printing 'error' twice here redundant?


More information about the webkit-reviews mailing list