mirror of
https://github.com/HDFGroup/hdf5.git
synced 2024-11-21 01:04:10 +08:00
301 lines
11 KiB
HTML
301 lines
11 KiB
HTML
<!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>
|