Anders says we should use built-in SQLite versioning instead of SchemaVersion table.
Created attachment 26370 [details] proposed patch
Comment on attachment 26370 [details] proposed patch > + if (static_cast<int>(sizeof(userVersionSQL)) < numBytes) > + ASSERT_NOT_REACHED(); This is a strange way to write an assertion. Are you just trying to avoid the unused local variable warning? Is there a better idiom for this sort of thing? It seems to come up often. r=me
Yes, that was the reason. Other ways to write this are to put the left part of variable definition/assignment under ifndef NDEBUG, or to add UNUSED_PARAM() for the variable. I'm not sure if either is better.
(In reply to comment #3) > Yes, that was the reason. Other ways to write this are to put the left part of > variable definition/assignment under ifndef NDEBUG, or to add UNUSED_PARAM() > for the variable. I'm not sure if either is better. This comes up often enough that I think we should add a macro just for this purpose: ASSERT_RESULT(variableName, result); It could use UNUSED_PARAM in NDEBUG and plain old ASSERT in debug. An advantage of this over ASSERT_NOT_REACHED is that the assertion message can be more specific. And also, clarity. It's way better than the #ifndef NDEBUG and the explicit use of UNUSED_PARAM for something that's not a "PARAM". What do you think?
Sounds good to me. I'll make a patch (do you think that adding this to wtf/UnusedParam.h would be acceptable? or should I rename it to Unused.h?)
(In reply to comment #5) > I'll make a patch (do you think that adding this to > wtf/UnusedParam.h would be acceptable? I think this is part of Assertions.h. The fact that it depends on UnusedParam.h is an implementation detail. > or should I rename it to Unused.h?) That might be good to do eventually.
(In reply to comment #6) > I think this is part of Assertions.h. The fact that it depends on UnusedParam.h > is an implementation detail. And whether it actually uses UnusedParam.h or not is an open question in my opinion.
Committed revision 39597. I'll make a separate patch for ASSERT_RESULT.
(In reply to comment #8) > Committed revision 39597. I'll make a separate patch for ASSERT_RESULT. I was also thinking we might want a similar ASSERT_UNUSED_PARAM function for an assertion on a parameter that is otherwise unused. Sorry to continue hijacking this bug report for this discussion.