Bug 23077 – Make application cache use SQLite built-in user_version

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alexey Proskuryakov]

Anders says we should use built-in SQLite versioning instead of SchemaVersion table.

[Alexey Proskuryakov]

Created attachment 26370 [details]
proposed patch

[Darin Adler]

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

[Alexey Proskuryakov]

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.

[Darin Adler]

(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?

[Alexey Proskuryakov]

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?)

[Darin Adler]

(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.

[Darin Adler]

(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.

[Alexey Proskuryakov]

Committed revision 39597. I'll make a separate patch for ASSERT_RESULT.

[Darin Adler]

(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.