diff options
Diffstat (limited to 'doc/coding-standard.tex')
| -rw-r--r-- | doc/coding-standard.tex | 73 |
1 files changed, 48 insertions, 25 deletions
diff --git a/doc/coding-standard.tex b/doc/coding-standard.tex index ed9d920ec..30d412a91 100644 --- a/doc/coding-standard.tex +++ b/doc/coding-standard.tex @@ -27,7 +27,7 @@ purpose. The comment should be descriptive without being overly verbose. \textsl{Good}: \begin{verbatim} - DBTYPE access_mode; /* access mode for accessing + DBTYPE access_mode; /* access mode for accessing * the databases, can be * DB_HASH, DB_BTREE * (option access-mode <mode>) @@ -69,7 +69,7 @@ Never use a non-constant expression as the initialization value for a variable. \section*{$\bullet$ Validate all arguments to a function} All pointer arguments to a function must be checked for \texttt{NULL}. -A macro named \texttt{VALIDATE} (in \texttt{common-utils.h}) +A macro named \texttt{VALIDATE} (in \texttt{common-utils.h}) takes one argument, and if it is \texttt{NULL}, writes a log message and jumps to a label called \texttt{err} after setting op\_ret and op\_errno appropriately. It is recommended to use this template. @@ -142,8 +142,8 @@ for success or failure. \begin{verbatim} op_ret = close (_fd); if (op_ret == -1) { - gf_log (this->name, GF_LOG_ERROR, - "close on file %s failed (%s)", real_path, + gf_log (this->name, GF_LOG_ERROR, + "close on file %s failed (%s)", real_path, strerror (errno)); op_errno = errno; goto out; @@ -157,9 +157,9 @@ memory allocation fails, the call should be unwound and an error returned to the user. \section*{$\bullet$ Use result args and reserve the return value to indicate success or failure} -The return value of every functions must indicate success or failure (unless -it is impossible for the function to fail --- e.g., boolean functions). If -the function needs to return additional data, it must be returned using a +The return value of every functions must indicate success or failure (unless +it is impossible for the function to fail --- e.g., boolean functions). If +the function needs to return additional data, it must be returned using a result (pointer) argument. \vspace{2ex} @@ -192,17 +192,21 @@ Unless impossible, use the length-limited versions of the string functions. \end{verbatim} \section*{$\bullet$ No dead or commented code} -There must be no dead code (code to which control can never be passed) or +There must be no dead code (code to which control can never be passed) or commented out code in the codebase. \section*{$\bullet$ Only one unwind and return per function} -There must be only one exit out of a function. \texttt{UNWIND} and return +There must be only one exit out of a function. \texttt{UNWIND} and return should happen at only point in the function. -\section*{$\bullet$ Keep functions small} -Try to keep functions small. Two to three screenfulls (80 lines per screen) is -considered a reasonable limit. If a function is very long, try splitting it -into many little helper functions. +\section*{$\bullet$ Function length or Keep functions small} +We live in the UNIX-world where modules do one thing and do it well. +This rule should apply to our functions also. If a function is very long, try splitting it +into many little helper functions. The question is, in a coding +spree, how do we know a function is long and unreadable. One rule of +thumb given by Linus Torvalds is that, a function should be broken-up +if you have 4 or more levels of indentation going on for more than 3-4 +lines. \vspace{2ex} \textsl{Example for a helper function}: @@ -215,6 +219,28 @@ into many little helper functions. } \end{verbatim} +\section*{$\bullet$ Defining functions as static} +Define internal functions as static only if you're +very sure that there will not be a crash(..of any kind..) emanating in +that function. If there is even a remote possibility, perhaps due to +pointer derefering, etc, declare the function as non-static. This +ensures that when a crash does happen, the function name shows up the +in the back-trace generated by libc. However, doing so has potential +for polluting the function namespace, so to avoid conflicts with other +components in other parts, ensure that the function names are +prepended with a prefix that identify the component to which it +belongs. For eg. non-static functions in io-threads translator start +with iot\_. + +\section*{$\bullet$ Ensure function calls wrap around after +80-columns.} +Place remaining arguments on the next line if needed. + +\section*{$\bullet$ Functions arguments and function definition} +Place all the arguments of a function definition on the same line +until the line goes beyond 80-cols. Arguments that extend beyind +80-cols should be placed on the next line. + \section*{Style issues} \subsection*{Brace placement} @@ -279,7 +305,7 @@ documentation. \end{verbatim} \subsection*{Indicating critical sections} -To clearly show regions of code which execute with locks held, use +To clearly show regions of code which execute with locks held, use the following format: \begin{verbatim} @@ -298,10 +324,8 @@ point, \texttt{out}. At that point, the code should detect the error that has occured and do appropriate cleanup. \begin{verbatim} -int32_t -sample_fop (call_frame_t *frame, - xlator_t *this, - ...) +int32_t +sample_fop (call_frame_t *frame, xlator_t *this, ...) { char * var1 = NULL; int32_t op_ret = -1; @@ -313,13 +337,13 @@ sample_fop (call_frame_t *frame, VALIDATE_OR_GOTO (this, out); /* other validations */ - + dir = opendir (...); if (dir == NULL) { op_errno = errno; - gf_log (this->name, GF_LOG_ERROR, - "opendir failed on %s (%s)", loc->path, + gf_log (this->name, GF_LOG_ERROR, + "opendir failed on %s (%s)", loc->path, strerror (op_errno)); goto out; } @@ -343,11 +367,10 @@ sample_fop (call_frame_t *frame, if (dir) { closedir (dir); dir = NULL; - } - + } + if (pfd) { - if (pfd->path) - FREE (pfd->path); + FREE (pfd->path); FREE (pfd); pfd = NULL; } |