1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
|
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
<html>
<head>
<title>Code Review</title>
</head>
<!--
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
* Copyright by The HDF Group. *
* Copyright by the Board of Trustees of the University of Illinois. *
* All rights reserved. *
* *
* This file is part of HDF5. The full HDF5 copyright notice, including *
* terms governing use, modification, and redistribution, is contained in *
* the files COPYING and Copyright.html. COPYING can be found at the root *
* of the source code distribution tree; Copyright.html can be found at the *
* root level of an installed copy of the electronic HDF5 document set and *
* is linked from the top-level documents page. It can also be found at *
* http://hdfgroup.org/HDF5/doc/Copyright.html. If you do not have *
* access to either file, you may request a copy from help@hdfgroup.org. *
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
-->
<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>
|
