diff options
Diffstat (limited to 'doc/html/TechNotes/CodeReview.html')
| -rw-r--r-- | doc/html/TechNotes/CodeReview.html | 300 |
1 files changed, 300 insertions, 0 deletions
diff --git a/doc/html/TechNotes/CodeReview.html b/doc/html/TechNotes/CodeReview.html new file mode 100644 index 0000000..213cbbe --- /dev/null +++ b/doc/html/TechNotes/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<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> |