<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
<title>Code Review</title>
</head>
<body>
<center><h1>Code Review 1</h1></center>
<h3>Some background...</h3>
<p>This is one of the functions exported from the
<code>H5B.c</code> file that implements a B-link-tree class
without worrying about concurrency yet (thus the `Note:' in the
function prologue). The <code>H5B.c</code> 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,
<code>H5G_stab_find()</code> takes a symbol table OID and a name
and calls <code>H5B_find()</code> with an appropriate
<code>udata</code> argument that eventually gets passed to the
<code>H5G_stab_find()</code> function.
<p><code><pre>
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 * matzke@llnl.gov
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;
</pre></code>
<p>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.
<p><code><pre>
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));
</pre></code>
<p>I use <code>assert</code> 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.
<p><code><pre>
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 }
</pre></code>
<p>You'll see this quite often in the low-level stuff and it's
documented in the <code>H5AC.c</code> file. The
<code>H5AC_protect</code> insures that the B-tree node (which
inherits from the H5AC package) whose OID is <code>addr</code>
is locked into memory for the duration of this function (see the
<code>H5AC_unprotect</code> 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 <code>H5AC_protect</code> 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.
<p>The alternative is to call the slighlty cheaper
<code>H5AC_find</code> 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
<em>must be unprotected</em> before the file is closed, thus the
use of <code>HGOTO_ERROR</code> instead of
<code>HRETURN_ERROR</code>.
<p><code><pre>
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 }
</pre></code>
<p>Code is arranged in paragraphs with a comment starting each
paragraph. The previous paragraph is a standard binary search
algorithm. The <code>(type->cmp3)()</code> 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).
<p>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 <code>=</code> instead
of <code>==</code>.
<p><code><pre>
72
73 /*
74 * Follow the link to the subtree or to the data node.
75 */
76 assert (idx>=0 && idx<bt->nchildren);
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 }
</pre></code>
<p>Here I broke the "side effect in conditional" rule, which I
sometimes do if the expression is so long that the
<code><0</code> gets lost at the end. Another thing to note is
that success/failure is always determined by comparing with zero
instead of <code>SUCCEED</code> or <code>FAIL</code>. 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).
<p><code><pre>
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 }
</pre></code>
<p>For lack of a better way to handle errors during error cleanup,
I just call the <code>HRETURN_ERROR</code> macro even though it
will make the error stack not quite right. I also use short
circuiting boolean operators instead of nested <code>if</code>
statements since that's standard C practice.
<center><h1>Code Review 2</h1></center>
<p>The following code is an API function from the H5F package...
<p><code><pre>
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 --------------------------------------------------------------------------*/
</pre></code>
<p>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.
<p><code><pre>
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;
</pre></code>
<p>API functions are never called internally, therefore I always
clear the error stack before doing anything.
<p><code><pre>
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 }
</pre></code>
<p>If something is wrong with the arguments then we raise an
error. We never <code>assert</code> 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 <code>hid_t</code>
type.
<p><code><pre>
44
45 /* do work */
46 if (H5F_flush (file, invalidate)<0) {
47 HRETURN_ERROR (H5E_CACHE, H5E_CANTFLUSH, FAIL); /*flush failed*/
48 }
</pre></code>
<p>An internal version of the function does the real work. That
internal version calls <code>assert</code> to check/document
it's arguments and can be called from other library functions.
<p><code><pre>
49
50 FUNC_LEAVE (SUCCEED);
51 }
</pre></code>
<hr>
<address><a href="mailto:matzke@llnl.gov">Robb Matzke</a></address>
<!-- Created: Sat Nov 8 17:09:33 EST 1997 -->
<!-- hhmts start -->
Last modified: Mon Nov 10 15:33:33 EST 1997
<!-- hhmts end -->
</body>
</html>