mirror of
https://gitlab.com/libvirt/libvirt.git
synced 2024-08-27 02:51:16 +00:00
* docs/hacking.html.in (Curly braces): Tighten recommendations to disallow if (cond) one-line; else { block; }. * HACKING: Regenerate. Suggested by Daniel P. Berrange.
680 lines
21 KiB
Plaintext
680 lines
21 KiB
Plaintext
-*- buffer-read-only: t -*- vi: set ro:
|
|
DO NOT EDIT THIS FILE! IT IS GENERATED AUTOMATICALLY!
|
|
|
|
|
|
|
|
Contributor guidelines
|
|
======================
|
|
|
|
|
|
|
|
General tips for contributing patches
|
|
=====================================
|
|
(1) Discuss any large changes on the mailing list first. Post patches early and
|
|
listen to feedback.
|
|
|
|
(2) Post patches in unified diff format. A command similar to this should work:
|
|
|
|
diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch
|
|
|
|
or:
|
|
|
|
git diff > libvirt-myfeature.patch
|
|
|
|
(3) Split large changes into a series of smaller patches, self-contained if
|
|
possible, with an explanation of each patch and an explanation of how the
|
|
sequence of patches fits together.
|
|
|
|
(4) Make sure your patches apply against libvirt GIT. Developers only follow GIT
|
|
and don't care much about released versions.
|
|
|
|
(5) Run the automated tests on your code before submitting any changes. In
|
|
particular, configure with compile warnings set to -Werror:
|
|
|
|
./configure --enable-compile-warnings=error
|
|
|
|
and run the tests:
|
|
|
|
make check
|
|
make syntax-check
|
|
make -C tests valgrind
|
|
|
|
The latter test checks for memory leaks.
|
|
|
|
If you encounter any failing tests, the VIR_TEST_DEBUG environment variable
|
|
may provide extra information to debug the failures. Larger values of
|
|
VIR_TEST_DEBUG may provide larger amounts of information:
|
|
|
|
VIR_TEST_DEBUG=1 make check (or)
|
|
VIR_TEST_DEBUG=2 make check
|
|
|
|
Also, individual tests can be run from inside the "tests/" directory, like:
|
|
|
|
./qemuxml2xmltest
|
|
|
|
(6) Update tests and/or documentation, particularly if you are adding a new
|
|
feature or changing the output of a program.
|
|
|
|
|
|
|
|
There is more on this subject, including lots of links to background reading
|
|
on the subject, on
|
|
|
|
Richard Jones' guide to working with open source projects
|
|
http://et.redhat.com/~rjones/how-to-supply-code-to-open-source-projects/
|
|
|
|
|
|
Code indentation
|
|
================
|
|
Libvirt's C source code generally adheres to some basic code-formatting
|
|
conventions. The existing code base is not totally consistent on this front,
|
|
but we do prefer that contributed code be formatted similarly. In short, use
|
|
spaces-not-TABs for indentation, use 4 spaces for each indentation level, and
|
|
other than that, follow the K&R style.
|
|
|
|
If you use Emacs, add the following to one of one of your start-up files
|
|
(e.g., ~/.emacs), to help ensure that you get indentation right:
|
|
|
|
;;; When editing C sources in libvirt, use this style.
|
|
(defun libvirt-c-mode ()
|
|
"C mode with adjusted defaults for use with libvirt."
|
|
(interactive)
|
|
(c-set-style "K&R")
|
|
(setq indent-tabs-mode nil) ; indent using spaces, not TABs
|
|
(setq c-indent-level 4)
|
|
(setq c-basic-offset 4))
|
|
(add-hook 'c-mode-hook
|
|
'(lambda () (if (string-match "/libvirt" (buffer-file-name))
|
|
(libvirt-c-mode))))
|
|
|
|
|
|
Code formatting (especially for new code)
|
|
=========================================
|
|
With new code, we can be even more strict. Please apply the following function
|
|
(using GNU indent) to any new code. Note that this also gives you an idea of
|
|
the type of spacing we prefer around operators and keywords:
|
|
|
|
indent-libvirt()
|
|
{
|
|
indent -bad -bap -bbb -bli4 -br -ce -brs -cs -i4 -l75 -lc75 \
|
|
-sbi4 -psl -saf -sai -saw -sbi4 -ss -sc -cdw -cli4 -npcs -nbc \
|
|
--no-tabs "$@"
|
|
}
|
|
|
|
Note that sometimes you'll have to post-process that output further, by piping
|
|
it through "expand -i", since some leading TABs can get through. Usually
|
|
they're in macro definitions or strings, and should be converted anyhow.
|
|
|
|
|
|
Curly braces
|
|
============
|
|
Omit the curly braces around an "if", "while", "for" etc. body only when that
|
|
body occupies a single line. In every other case we require the braces. This
|
|
ensures that it is trivially easy to identify a single-'statement' loop: each
|
|
has only one 'line' in its body.
|
|
|
|
Omitting braces with a single-line body is fine:
|
|
|
|
while (expr) // one-line body -> omitting curly braces is ok
|
|
single_line_stmt();
|
|
|
|
However, the moment your loop/if/else body extends onto a second line, for
|
|
whatever reason (even if it's just an added comment), then you should add
|
|
braces. Otherwise, it would be too easy to insert a statement just before that
|
|
comment (without adding braces), thinking it is already a multi-statement loop:
|
|
|
|
while (true) // BAD! multi-line body with no braces
|
|
/* comment... */
|
|
single_line_stmt();
|
|
|
|
Do this instead:
|
|
|
|
while (true) { // Always put braces around a multi-line body.
|
|
/* comment... */
|
|
single_line_stmt();
|
|
}
|
|
|
|
There is one exception: when the second body line is not at the same
|
|
indentation level as the first body line:
|
|
|
|
if (expr)
|
|
die("a diagnostic that would make this line"
|
|
" extend past the 80-column limit"));
|
|
|
|
It is safe to omit the braces in the code above, since the further-indented
|
|
second body line makes it obvious that this is still a single-statement body.
|
|
|
|
To reiterate, don't do this:
|
|
|
|
if (expr) // BAD: no braces around...
|
|
while (expr_2) { // ... a multi-line body
|
|
...
|
|
}
|
|
|
|
Do this, instead:
|
|
|
|
if (expr) {
|
|
while (expr_2) {
|
|
...
|
|
}
|
|
}
|
|
|
|
However, there is one exception in the other direction, when even a one-line
|
|
block should have braces. That occurs when that one-line, brace-less block is
|
|
an "if" or "else" block, and the counterpart block *does* use braces. In that
|
|
case, put braces around both blocks. Also, if the "else" block is much shorter
|
|
than the "if" block, consider negating the "if"-condition and swapping the
|
|
bodies, putting the short block first and making the longer, multi-line block
|
|
be the "else" block.
|
|
|
|
if (expr) {
|
|
...
|
|
...
|
|
}
|
|
else
|
|
x = y; // BAD: braceless "else" with braced "then",
|
|
// and short block last
|
|
|
|
if (expr)
|
|
x = y; // BAD: braceless "if" with braced "else"
|
|
else {
|
|
...
|
|
...
|
|
}
|
|
|
|
Keeping braces consistent and putting the short block first is preferred,
|
|
especially when the multi-line body is more than a few lines long, because it
|
|
is easier to read and grasp the semantics of an if-then-else block when the
|
|
simpler block occurs first, rather than after the more involved block:
|
|
|
|
if (!expr) {
|
|
x = y; // putting the smaller block first is more readable
|
|
} else {
|
|
...
|
|
...
|
|
}
|
|
|
|
But if negating a complex condition is too ugly, then at least add braces:
|
|
|
|
if (complex expr not worth negating) {
|
|
...
|
|
...
|
|
} else {
|
|
x = y;
|
|
}
|
|
|
|
|
|
Preprocessor
|
|
============
|
|
For variadic macros, stick with C99 syntax:
|
|
|
|
#define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
|
|
|
|
Use parenthesis when checking if a macro is defined, and use indentation to
|
|
track nesting:
|
|
|
|
#if defined(HAVE_POSIX_FALLOCATE) && !defined(HAVE_FALLOCATE)
|
|
# define fallocate(a,ignored,b,c) posix_fallocate(a,b,c)
|
|
#endif
|
|
|
|
|
|
C types
|
|
=======
|
|
Use the right type.
|
|
|
|
Scalars
|
|
-------
|
|
- If you're using "int" or "long", odds are good that there's a better type.
|
|
|
|
- If a variable is counting something, be sure to declare it with an unsigned
|
|
type.
|
|
|
|
- If it's memory-size-related, use "size_t" (use "ssize_t" only if required).
|
|
|
|
- If it's file-size related, use uintmax_t, or maybe "off_t".
|
|
|
|
- If it's file-offset related (i.e., signed), use "off_t".
|
|
|
|
- If it's just counting small numbers use "unsigned int"; (on all but oddball
|
|
embedded systems, you can assume that that type is at least four bytes wide).
|
|
|
|
- If a variable has boolean semantics, give it the "bool" type and use the
|
|
corresponding "true" and "false" macros. It's ok to include <stdbool.h>, since
|
|
libvirt's use of gnulib ensures that it exists and is usable.
|
|
|
|
- In the unusual event that you require a specific width, use a standard type
|
|
like "int32_t", "uint32_t", "uint64_t", etc.
|
|
|
|
- While using "bool" is good for readability, it comes with minor caveats:
|
|
|
|
-- Don't use "bool" in places where the type size must be constant across all
|
|
systems, like public interfaces and on-the-wire protocols. Note that it would
|
|
be possible (albeit wasteful) to use "bool" in libvirt's logical wire
|
|
protocol, since XDR maps that to its lower-level "bool_t" type, which *is*
|
|
fixed-size.
|
|
|
|
-- Don't compare a bool variable against the literal, "true", since a value with
|
|
a logical non-false value need not be "1". I.e., don't write "if (seen ==
|
|
true) ...". Rather, write "if (seen)...".
|
|
|
|
|
|
|
|
|
|
|
|
Of course, take all of the above with a grain of salt. If you're about to use
|
|
some system interface that requires a type like "size_t", "pid_t" or "off_t",
|
|
use matching types for any corresponding variables.
|
|
|
|
Also, if you try to use e.g., "unsigned int" as a type, and that conflicts
|
|
with the signedness of a related variable, sometimes it's best just to use the
|
|
*wrong* type, if 'pulling the thread' and fixing all related variables would
|
|
be too invasive.
|
|
|
|
Finally, while using descriptive types is important, be careful not to go
|
|
overboard. If whatever you're doing causes warnings, or requires casts, then
|
|
reconsider or ask for help.
|
|
|
|
Pointers
|
|
--------
|
|
Ensure that all of your pointers are 'const-correct'. Unless a pointer is used
|
|
to modify the pointed-to storage, give it the "const" attribute. That way, the
|
|
reader knows up-front that this is a read-only pointer. Perhaps more
|
|
importantly, if we're diligent about this, when you see a non-const pointer,
|
|
you're guaranteed that it is used to modify the storage it points to, or it is
|
|
aliased to another pointer that is.
|
|
|
|
|
|
Low level memory management
|
|
===========================
|
|
Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt
|
|
codebase, because they encourage a number of serious coding bugs and do not
|
|
enable compile time verification of checks for NULL. Instead of these
|
|
routines, use the macros from memory.h.
|
|
|
|
- To allocate a single object:
|
|
|
|
virDomainPtr domain;
|
|
|
|
if (VIR_ALLOC(domain) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
|
|
|
|
|
|
- To allocate an array of objects:
|
|
|
|
virDomainPtr domains;
|
|
size_t ndomains = 10;
|
|
|
|
if (VIR_ALLOC_N(domains, ndomains) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
|
|
|
|
|
|
- To allocate an array of object pointers:
|
|
|
|
virDomainPtr *domains;
|
|
size_t ndomains = 10;
|
|
|
|
if (VIR_ALLOC_N(domains, ndomains) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
|
|
|
|
|
|
- To re-allocate the array of domains to be 1 element longer (however, note that
|
|
repeatedly expanding an array by 1 scales quadratically, so this is
|
|
recommended only for smaller arrays):
|
|
|
|
virDomainPtr domains;
|
|
size_t ndomains = 0;
|
|
|
|
if (VIR_EXPAND_N(domains, ndomains, 1) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
domains[ndomains - 1] = domain;
|
|
|
|
|
|
|
|
- To ensure an array has room to hold at least one more element (this approach
|
|
scales better, but requires tracking allocation separately from usage)
|
|
|
|
virDomainPtr domains;
|
|
size_t ndomains = 0;
|
|
size_t ndomains_max = 0;
|
|
|
|
if (VIR_RESIZE_N(domains, ndomains_max, ndomains, 1) < 0) {
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
domains[ndomains++] = domain;
|
|
|
|
|
|
|
|
- To trim an array of domains from its allocated size down to the actual used
|
|
size:
|
|
|
|
virDomainPtr domains;
|
|
size_t ndomains = x;
|
|
size_t ndomains_max = y;
|
|
|
|
VIR_SHRINK_N(domains, ndomains_max, ndomains_max - ndomains);
|
|
|
|
|
|
|
|
- To free an array of domains:
|
|
|
|
virDomainPtr domains;
|
|
size_t ndomains = x;
|
|
size_t ndomains_max = y;
|
|
size_t i;
|
|
|
|
for (i = 0; i < ndomains; i++)
|
|
VIR_FREE(domains[i]);
|
|
VIR_FREE(domains);
|
|
ndomains_max = ndomains = 0;
|
|
|
|
|
|
|
|
|
|
|
|
|
|
File handling
|
|
=============
|
|
Usage of the "fdopen()", "close()", "fclose()" APIs is deprecated in libvirt
|
|
code base to help avoiding double-closing of files or file descriptors, which
|
|
is particulary dangerous in a multi-threaded applications. Instead of these
|
|
APIs, use the macros from files.h
|
|
|
|
- Open a file from a file descriptor:
|
|
|
|
if ((file = VIR_FDOPEN(fd, "r")) == NULL) {
|
|
virReportSystemError(errno, "%s",
|
|
_("failed to open file from file descriptor"));
|
|
return -1;
|
|
}
|
|
/* fd is now invalid; only access the file using file variable */
|
|
|
|
|
|
|
|
- Close a file descriptor:
|
|
|
|
if (VIR_CLOSE(fd) < 0) {
|
|
virReportSystemError(errno, "%s", _("failed to close file"));
|
|
}
|
|
|
|
|
|
|
|
- Close a file:
|
|
|
|
if (VIR_FCLOSE(file) < 0) {
|
|
virReportSystemError(errno, "%s", _("failed to close file"));
|
|
}
|
|
|
|
|
|
|
|
- Close a file or file descriptor in an error path, without losing the previous
|
|
"errno" value:
|
|
|
|
VIR_FORCE_CLOSE(fd);
|
|
VIR_FORCE_FCLOSE(file);
|
|
|
|
|
|
|
|
|
|
|
|
|
|
String comparisons
|
|
==================
|
|
Do not use the strcmp, strncmp, etc functions directly. Instead use one of the
|
|
following semantically named macros
|
|
|
|
- For strict equality:
|
|
|
|
STREQ(a,b)
|
|
STRNEQ(a,b)
|
|
|
|
|
|
|
|
- For case insensitive equality:
|
|
|
|
STRCASEEQ(a,b)
|
|
STRCASENEQ(a,b)
|
|
|
|
|
|
|
|
- For strict equality of a substring:
|
|
|
|
STREQLEN(a,b,n)
|
|
STRNEQLEN(a,b,n)
|
|
|
|
|
|
|
|
- For case insensitive equality of a substring:
|
|
|
|
STRCASEEQLEN(a,b,n)
|
|
STRCASENEQLEN(a,b,n)
|
|
|
|
|
|
|
|
- For strict equality of a prefix:
|
|
|
|
STRPREFIX(a,b)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
String copying
|
|
==============
|
|
Do not use the strncpy function. According to the man page, it does *not*
|
|
guarantee a NULL-terminated buffer, which makes it extremely dangerous to use.
|
|
Instead, use one of the functionally equivalent functions:
|
|
|
|
virStrncpy(char *dest, const char *src, size_t n, size_t destbytes)
|
|
|
|
The first three arguments have the same meaning as for strncpy; namely the
|
|
destination, source, and number of bytes to copy, respectively. The last
|
|
argument is the number of bytes available in the destination string; if a copy
|
|
of the source string (including a \0) will not fit into the destination, no
|
|
bytes are copied and the routine returns NULL. Otherwise, n bytes from the
|
|
source are copied into the destination and a trailing \0 is appended.
|
|
|
|
virStrcpy(char *dest, const char *src, size_t destbytes)
|
|
|
|
Use this variant if you know you want to copy the entire src string into dest.
|
|
Note that this is a macro, so arguments could be evaluated more than once.
|
|
This is equivalent to virStrncpy(dest, src, strlen(src), destbytes)
|
|
|
|
virStrcpyStatic(char *dest, const char *src)
|
|
|
|
Use this variant if you know you want to copy the entire src string into dest
|
|
*and* you know that your destination string is a static string (i.e. that
|
|
sizeof(dest) returns something meaningful). Note that this is a macro, so
|
|
arguments could be evaluated more than once. This is equivalent to
|
|
virStrncpy(dest, src, strlen(src), sizeof(dest)).
|
|
|
|
|
|
Variable length string buffer
|
|
=============================
|
|
If there is a need for complex string concatenations, avoid using the usual
|
|
sequence of malloc/strcpy/strcat/snprintf functions and make use of the
|
|
virBuffer API described in buf.h
|
|
|
|
Typical usage is as follows:
|
|
|
|
char *
|
|
somefunction(...)
|
|
{
|
|
virBuffer buf = VIR_BUFFER_INITIALIZER;
|
|
|
|
...
|
|
|
|
virBufferAddLit(&buf, "<domain>\n");
|
|
virBufferVSprintf(&buf, " <memory>%d</memory>\n", memory);
|
|
...
|
|
virBufferAddLit(&buf, "</domain>\n");
|
|
|
|
...
|
|
|
|
if (virBufferError(&buf)) {
|
|
virBufferFreeAndReset(&buf);
|
|
virReportOOMError();
|
|
return NULL;
|
|
}
|
|
|
|
return virBufferContentAndReset(&buf);
|
|
}
|
|
|
|
|
|
Include files
|
|
=============
|
|
There are now quite a large number of include files, both libvirt internal and
|
|
external, and system includes. To manage all this complexity it's best to
|
|
stick to the following general plan for all *.c source files:
|
|
|
|
/*
|
|
* Copyright notice
|
|
* ....
|
|
* ....
|
|
* ....
|
|
*
|
|
*/
|
|
|
|
#include <config.h> Must come first in every file.
|
|
|
|
#include <stdio.h> Any system includes you need.
|
|
#include <string.h>
|
|
#include <limits.h>
|
|
|
|
#if HAVE_NUMACTL Some system includes aren't supported
|
|
# include <numa.h> everywhere so need these #if guards.
|
|
#endif
|
|
|
|
#include "internal.h" Include this first, after system includes.
|
|
|
|
#include "util.h" Any libvirt internal header files.
|
|
#include "buf.h"
|
|
|
|
static int
|
|
myInternalFunc() The actual code.
|
|
{
|
|
...
|
|
|
|
Of particular note: *Do not* include libvirt/libvirt.h or libvirt/virterror.h.
|
|
It is included by "internal.h" already and there are some special reasons why
|
|
you cannot include these files explicitly.
|
|
|
|
|
|
Printf-style functions
|
|
======================
|
|
Whenever you add a new printf-style function, i.e., one with a format string
|
|
argument and following "..." in its prototype, be sure to use gcc's printf
|
|
attribute directive in the prototype. For example, here's the one for
|
|
virAsprintf, in util.h:
|
|
|
|
int virAsprintf(char **strp, const char *fmt, ...)
|
|
ATTRIBUTE_FORMAT(printf, 2, 3);
|
|
|
|
This makes it so gcc's -Wformat and -Wformat-security options can do their
|
|
jobs and cross-check format strings with the number and types of arguments.
|
|
|
|
When printing to a string, consider using virBuffer for incremental
|
|
allocations, virAsprintf for a one-shot allocation, and snprintf for
|
|
fixed-width buffers. Do not use sprintf, even if you can prove the buffer
|
|
won't overflow, since gnulib does not provide the same portability guarantees
|
|
for sprintf as it does for snprintf.
|
|
|
|
|
|
Use of goto
|
|
===========
|
|
The use of goto is not forbidden, and goto is widely used throughout libvirt.
|
|
While the uncontrolled use of goto will quickly lead to unmaintainable code,
|
|
there is a place for it in well structured code where its use increases
|
|
readability and maintainability. In general, if goto is used for error
|
|
recovery, it's likely to be ok, otherwise, be cautious or avoid it all
|
|
together.
|
|
|
|
The typical use of goto is to jump to cleanup code in the case of a long list
|
|
of actions, any of which may fail and cause the entire operation to fail. In
|
|
this case, a function will have a single label at the end of the function.
|
|
It's almost always ok to use this style. In particular, if the cleanup code
|
|
only involves free'ing memory, then having multiple labels is overkill.
|
|
VIR_FREE() and every function named XXXFree() in libvirt is required to handle
|
|
NULL as its arg. Thus you can safely call free on all the variables even if
|
|
they were not yet allocated (yes they have to have been initialized to NULL).
|
|
This is much simpler and clearer than having multiple labels.
|
|
|
|
There are a couple of signs that a particular use of goto is not ok:
|
|
|
|
- You're using multiple labels. If you find yourself using multiple labels,
|
|
you're strongly encouraged to rework your code to eliminate all but one of
|
|
them.
|
|
|
|
- The goto jumps back up to a point above the current line of code being
|
|
executed. Please use some combination of looping constructs to re-execute code
|
|
instead; it's almost certainly going to be more understandable by others. One
|
|
well-known exception to this rule is restarting an i/o operation following
|
|
EINTR.
|
|
|
|
- The goto jumps down to an arbitrary place in the middle of a function followed
|
|
by further potentially failing calls. You should almost certainly be using a
|
|
conditional and a block instead of a goto. Perhaps some of your function's
|
|
logic would be better pulled out into a helper function.
|
|
|
|
|
|
|
|
Although libvirt does not encourage the Linux kernel wind/unwind style of
|
|
multiple labels, there's a good general discussion of the issue archived at
|
|
|
|
KernelTrap
|
|
http://kerneltrap.org/node/553/2131
|
|
|
|
When using goto, please use one of these standard labels if it makes sense:
|
|
|
|
error: A path only taken upon return with an error code
|
|
cleanup: A path taken upon return with success code + optional error
|
|
no_memory: A path only taken upon return with an OOM error code
|
|
retry: If needing to jump upwards (e.g., retry on EINTR)
|
|
|
|
|
|
Libvirt committer guidelines
|
|
============================
|
|
The AUTHORS files indicates the list of people with commit access right who
|
|
can actually merge the patches.
|
|
|
|
The general rule for committing a patch is to make sure it has been reviewed
|
|
properly in the mailing-list first, usually if a couple of people gave an ACK
|
|
or +1 to a patch and nobody raised an objection on the list it should be good
|
|
to go. If the patch touches a part of the code where you're not the main
|
|
maintainer, or where you do not have a very clear idea of how things work,
|
|
it's better to wait for a more authoritative feedback though. Before
|
|
committing, please also rebuild locally, run 'make check syntax-check', and
|
|
make sure you don't raise errors. Try to look for warnings too; for example,
|
|
configure with
|
|
|
|
--enable-compile-warnings=error
|
|
|
|
which adds -Werror to compile flags, so no warnings get missed
|
|
|
|
An exception to 'review and approval on the list first' is fixing failures to
|
|
build:
|
|
|
|
- if a recently committed patch breaks compilation on a platform or for a given
|
|
driver, then it's fine to commit a minimal fix directly without getting the
|
|
review feedback first
|
|
|
|
- if make check or make syntax-check breaks, if there is an obvious fix, it's
|
|
fine to commit immediately. The patch should still be sent to the list (or
|
|
tell what the fix was if trivial), and 'make check syntax-check' should pass
|
|
too, before committing anything
|
|
|
|
- fixes for documentation and code comments can be managed in the same way, but
|
|
still make sure they get reviewed if non-trivial.
|