cruppstahl/hamsterdb — pull request #9 by GerHobbelt (2012-08-21 16:29:46+00:00)
Title: passing TXN+HINTS type flags int to db_fetch_page: wrong

while doing a bit of merge work on hamster (we'll get to that later, sigh), I ran a couple of review-based flow analysis checks, as my old assert were disappearing and I know db_fetch_page() was nasty enough to bite my ass at very odd moments, thanks to the mixed return behaviour (depending on flags, page can be _legally_ NULL).

Here we see an occasion where its flags SHOULD be 0 and MUST NOT be a via-via copy of the cursor::erase flags, as _that_ one includes both HAM_HINT_* flag bits and TXN/ERASE specific flag bits (rather: bit), and it just so happens that the hint flag bits sit in the same bit region as the db_fetch_page() flag bits, and more specifically DB_ONLY_FROM_CACHE.

Thinking out loud here:

The wrong approach to fixing this would be to put those in different bit positions, as they should never appear in the same flags int ever anyhow -- one may consider using special crafted enum types for these flags int types; you moved several hint flags to internal-use-only while they really aren't ;-), so nothing stands in the way of introducing a couple of enum types to make the compiler help us detect 'flags' misuse.

Though the mixing of hints + find + erase flag bits in a single enum type -- as it has to be, or the hint bits must be duplicated for insert, find and erase flag enums and then you'ld have some enum collisions inside the btree logic, so we can conclude after all that 'enum types' isn't the golden egg here either and isn't even expected to help us make it _easier_ for ourselves, now that I think about it a bit more.
So 'int' they should remain and the buck is passed to flow review/analysis by mind. :-)