Code Review 1

Some background...

This is one of the functions exported from the H5B.c file that implements a B-link-tree class without worrying about concurrency yet (thus the `Note:' in the function prologue). The H5B.c file provides the basic machinery for operating on generic B-trees, but it isn't much use by itself. Various subclasses of the B-tree (like symbol tables or indirect storage) provide their own interface and back end to this function. For instance, H5G_stab_find() takes a symbol table OID and a name and calls H5B_find() with an appropriate udata argument that eventually gets passed to the H5G_stab_find() function.

 1 /*-------------------------------------------------------------------------
 2  * Function:    H5B_find
 3  *
 4  * Purpose:     Locate the specified information in a B-tree and return
 5  *              that information by filling in fields of the caller-supplied
 6  *              UDATA pointer depending on the type of leaf node
 7  *              requested.  The UDATA can point to additional data passed
 8  *              to the key comparison function.
 9  *
10  * Note:        This function does not follow the left/right sibling
11  *              pointers since it assumes that all nodes can be reached
12  *              from the parent node.
13  *
14  * Return:      Success:        SUCCEED if found, values returned through the
15  *                              UDATA argument.
16  *
17  *              Failure:        FAIL if not found, UDATA is undefined.
18  *
19  * Programmer:  Robb Matzke
20  *              [email protected]
21  *              Jun 23 1997
22  *
23  * Modifications:
24  *
25  *-------------------------------------------------------------------------
26  */
27 herr_t
28 H5B_find (H5F_t *f, const H5B_class_t *type, const haddr_t *addr, void *udata)
29 {
30    H5B_t        *bt=NULL;
31    intn         idx=-1, lt=0, rt, cmp=1;
32    int          ret_value = FAIL;
    

All pointer arguments are initialized when defined. I don't worry much about non-pointers because it's usually obvious when the value isn't initialized.

33 
34    FUNC_ENTER (H5B_find, NULL, FAIL);
35 
36    /*
37     * Check arguments.
38     */
39    assert (f);
40    assert (type);
41    assert (type->decode);
42    assert (type->cmp3);
43    assert (type->found);
44    assert (addr && H5F_addr_defined (addr));
    

I use assert to check invariant conditions. At this level of the library, none of these assertions should fail unless something is majorly wrong. The arguments should have already been checked by higher layers. It also provides documentation about what arguments might be optional.

45    
46    /*
47     * Perform a binary search to locate the child which contains
48     * the thing for which we're searching.
49     */
50    if (NULL==(bt=H5AC_protect (f, H5AC_BT, addr, type, udata))) {
51       HGOTO_ERROR (H5E_BTREE, H5E_CANTLOAD, FAIL);
52    }
    

You'll see this quite often in the low-level stuff and it's documented in the H5AC.c file. The H5AC_protect insures that the B-tree node (which inherits from the H5AC package) whose OID is addr is locked into memory for the duration of this function (see the H5AC_unprotect on line 90). Most likely, if this node has been accessed in the not-to-distant past, it will still be in memory and the H5AC_protect is almost a no-op. If cache debugging is compiled in, then the protect also prevents other parts of the library from accessing the node while this function is protecting it, so this function can allow the node to be in an inconsistent state while calling other parts of the library.

The alternative is to call the slighlty cheaper H5AC_find and assume that the pointer it returns is valid only until some other library function is called, but since we're accessing the pointer throughout this function, I chose to use the simpler protect scheme. All protected objects must be unprotected before the file is closed, thus the use of HGOTO_ERROR instead of HRETURN_ERROR.

53    rt = bt->nchildren;
54 
55    while (lt<rt && cmp) {
56       idx = (lt + rt) / 2;
57       if (H5B_decode_keys (f, bt, idx)<0) {
58          HGOTO_ERROR (H5E_BTREE, H5E_CANTDECODE, FAIL);
59       }
60 
61       /* compare */
62       if ((cmp=(type->cmp3)(f, bt->key[idx].nkey, udata,
63                             bt->key[idx+1].nkey))<0) {
64          rt = idx;
65       } else {
66          lt = idx+1;
67       }
68    }
69    if (cmp) {
70       HGOTO_ERROR (H5E_BTREE, H5E_NOTFOUND, FAIL);
71    }
    

Code is arranged in paragraphs with a comment starting each paragraph. The previous paragraph is a standard binary search algorithm. The (type->cmp3)() is an indirect function call into the subclass of the B-tree. All indirect function calls have the function part in parentheses to document that it's indirect (quite obvious here, but not so obvious when the function is a variable).

It's also my standard practice to have side effects in conditional expressions because I can write code faster and it's more apparent to me what the condition is testing. But if I have an assignment in a conditional expr, then I use an extra set of parens even if they're not required (usually they are, as in this case) so it's clear that I meant = instead of ==.

72 
73    /*
74     * Follow the link to the subtree or to the data node.
75     */
76    assert (idx>=0 && idxnchildren);
77    if (bt->level > 0) {
78       if ((ret_value = H5B_find (f, type, bt->child+idx, udata))<0) {
79          HGOTO_ERROR (H5E_BTREE, H5E_NOTFOUND, FAIL);
80       }
81    } else {
82       ret_value = (type->found)(f, bt->child+idx, bt->key[idx].nkey,
83                                 udata, bt->key[idx+1].nkey);
84       if (ret_value<0) {
85          HGOTO_ERROR (H5E_BTREE, H5E_NOTFOUND, FAIL);
86       }
87    }
    

Here I broke the "side effect in conditional" rule, which I sometimes do if the expression is so long that the <0 gets lost at the end. Another thing to note is that success/failure is always determined by comparing with zero instead of SUCCEED or FAIL. I do this because occassionally one might want to return other meaningful values (always non-negative) or distinguish between various types of failure (always negative).

88 
89 done:
90    if (bt && H5AC_unprotect (f, H5AC_BT, addr, bt)<0) {
91       HRETURN_ERROR (H5E_BTREE, H5E_PROTECT, FAIL);
92    }
93    FUNC_LEAVE (ret_value);
94 }
    

For lack of a better way to handle errors during error cleanup, I just call the HRETURN_ERROR macro even though it will make the error stack not quite right. I also use short circuiting boolean operators instead of nested if statements since that's standard C practice.

Code Review 2

The following code is an API function from the H5F package...

 1 /*--------------------------------------------------------------------------
 2  NAME
 3     H5Fflush
 4 
 5  PURPOSE
 6     Flush all cached data to disk and optionally invalidates all cached
 7     data.
 8 
 9  USAGE
10     herr_t H5Fflush(fid, invalidate)
11         hid_t fid;              IN: File ID of file to close.
12         hbool_t invalidate;     IN: Invalidate all of the cache?
13 
14  ERRORS
15     ARGS      BADTYPE       Not a file atom. 
16     ATOM      BADATOM       Can't get file struct. 
17     CACHE     CANTFLUSH     Flush failed. 
18 
19  RETURNS
20     SUCCEED/FAIL
21 
22  DESCRIPTION
23         This function flushes all cached data to disk and, if INVALIDATE
24     is non-zero, removes cached objects from the cache so they must be
25     re-read from the file on the next access to the object.
26 
27  MODIFICATIONS:
28 --------------------------------------------------------------------------*/
    

An API prologue is used for each API function instead of my normal function prologue. I use the prologue from Code Review 1 for non-API functions because it's more suited to C programmers, it requires less work to keep it synchronized with the code, and I have better editing tools for it.

29 herr_t
30 H5Fflush (hid_t fid, hbool_t invalidate)
31 {
32    H5F_t        *file = NULL;
33 
34    FUNC_ENTER (H5Fflush, H5F_init_interface, FAIL);
35    H5ECLEAR;
    

API functions are never called internally, therefore I always clear the error stack before doing anything.

36 
37    /* check arguments */
38    if (H5_FILE!=H5Aatom_group (fid)) {
39       HRETURN_ERROR (H5E_ARGS, H5E_BADTYPE, FAIL); /*not a file atom*/
40    }
41    if (NULL==(file=H5Aatom_object (fid))) {
42       HRETURN_ERROR (H5E_ATOM, H5E_BADATOM, FAIL); /*can't get file struct*/
43    }
    

If something is wrong with the arguments then we raise an error. We never assert arguments at this level. We also convert atoms to pointers since atoms are really just a pointer-hiding mechanism. Functions that can be called internally always have pointer arguments instead of atoms because (1) then they don't have to always convert atoms to pointers, and (2) the various pointer data types provide more documentation and type checking than just an hid_t type.

44 
45    /* do work */
46    if (H5F_flush (file, invalidate)<0) {
47       HRETURN_ERROR (H5E_CACHE, H5E_CANTFLUSH, FAIL); /*flush failed*/
48    }
    

An internal version of the function does the real work. That internal version calls assert to check/document it's arguments and can be called from other library functions.

49 
50    FUNC_LEAVE (SUCCEED);
51 }
    

Robb Matzke
Last modified: Mon Nov 10 15:33:33 EST 1997