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