summaryrefslogtreecommitdiffstats
path: root/doc/html/CodeReview.html
diff options
context:
space:
mode:
authorQuincey Koziol <koziol@hdfgroup.org>1998-07-08 14:54:54 (GMT)
committerQuincey Koziol <koziol@hdfgroup.org>1998-07-08 14:54:54 (GMT)
commitbd1e676c521d881b3143829f493a28b5ced1294b (patch)
tree69c50f9fe21ce87f293d8617a6bd51b4cc1e0244 /doc/html/CodeReview.html
parent73345095897d9698bb1f2f7df830bf80a56dc65a (diff)
downloadhdf5-bd1e676c521d881b3143829f493a28b5ced1294b.zip
hdf5-bd1e676c521d881b3143829f493a28b5ced1294b.tar.gz
hdf5-bd1e676c521d881b3143829f493a28b5ced1294b.tar.bz2
[svn-r467] Restructuring documentation.
Diffstat (limited to 'doc/html/CodeReview.html')
-rw-r--r--doc/html/CodeReview.html300
1 files changed, 300 insertions, 0 deletions
diff --git a/doc/html/CodeReview.html b/doc/html/CodeReview.html
new file mode 100644
index 0000000..213cbbe
--- /dev/null
+++ b/doc/html/CodeReview.html
@@ -0,0 +1,300 @@
+<!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&lt;rt && cmp) {
+56 idx = (lt + rt) / 2;
+57 if (H5B_decode_keys (f, bt, idx)&lt;0) {
+58 HGOTO_ERROR (H5E_BTREE, H5E_CANTDECODE, FAIL);
+59 }
+60
+61 /* compare */
+62 if ((cmp=(type-&gt;cmp3)(f, bt->key[idx].nkey, udata,
+63 bt->key[idx+1].nkey))&lt;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-&gt;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&gt;=0 && idx<bt->nchildren);
+77 if (bt->level > 0) {
+78 if ((ret_value = H5B_find (f, type, bt->child+idx, udata))&lt;0) {
+79 HGOTO_ERROR (H5E_BTREE, H5E_NOTFOUND, FAIL);
+80 }
+81 } else {
+82 ret_value = (type-&gt;found)(f, bt->child+idx, bt->key[idx].nkey,
+83 udata, bt->key[idx+1].nkey);
+84 if (ret_value&lt;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>&lt;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)&lt;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)&lt;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>