summaryrefslogtreecommitdiffstats
path: root/doc/html/TechNotes/CodeReview.html
blob: 213cbbe6ebfd90a4f9d988a5ff500e451a928036 (plain)
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
<!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>