Re: [PATCH] trace-cmd: Add CODING_STYLE and CONTRIBUTE documents

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

Hi Steven,
I have a few suggestions:

On Tue, Apr 6, 2021 at 10:41 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> [
>    Hi Sameer,
>    I added you since you are the newest contributor, and would like to know
>    if from your perspective, if these documents would have been helpful
>    to you when you first started to contribute.
>    Thanks!
> ]
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> Add a CODING_STYLE document that describes the expected style of the code
> as well as a CONTRIBUTE document that explains how a new comer can
> contribute to the project.
>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  CODING_STYLE | 179 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  CONTRIBUTE   |  80 +++++++++++++++++++++++
>  2 files changed, 259 insertions(+)
>  create mode 100644 CODING_STYLE
>  create mode 100644 CONTRIBUTE
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> new file mode 100644
> index 00000000..6aff138e
> --- /dev/null
> +++ b/CODING_STYLE
> @@ -0,0 +1,179 @@
> +
> +trace-cmd coding-style
> +======================
> +
> +The coding style of trace-cmd and the tracing librariers (libtracefs and
> +libtraceevent) are very similar to the Linux kernel coding style:
> +
> +  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst
> +
> +Indentation
> +===========
> +
> +Tabs are used for the start of indentation (the '\t' character), and should be
> +set to 8 characters. Spaces may be used at the end for continued lines where
> +having the start of text line up to braces in the previous line is not
> +divisible by 8.
> +
> +Max line width
> +==============

The desirable max line width should be written here, should we stick
with the 100 char limit, recently introduced in the Linux kernel ?

> +
> +This is a guide, as readability is more important than breaking lines up into a
> +hard limit. Ideally, strings should never be broken up except for where a new
> +line is added.
> +
> +       printf("This is a line that may continue for a very long string.\n"
> +              "This is another line, but after a new line\n");
> +
> +But line breaks should not be:
> +
> +       printf("This is a line that may continue for a very"
> +              "long string.\n This is another line,"
> +              "but after a new line\n");
> +
> +Not only is the above not as readable as the first version, it is not
> +even equivalent, because it is missing spaces between the line breaks.
> +For this reason, finish the string on the same line.
> +
> +Brackets and baces

a typo, should be "braces".

> +==================
> +
> +For all conditionals, the braces start on the same line:
> +
> +       if (cond) {
> +       }

That extra space before the opening brace should be mentioned in this
paragraph. As this is something usual for the Linux kernel developers,
I think it is not widely used by user space C developers.

> +
> +And the ending brace is at the same indentation as the conditional.
> +
> +       while (cond) {
> +       }
> +
> +       do {
> +       } while (cond);
> +
> +       for (i = 0; i < 10; i++) {
> +       }
> +
> +The same is true for structures:
> +
> +       struct my_struct {
> +               int field;
> +       };
> +
> +But for functions, the baces should start on the following line:
> +
> +       void my_function(void)
> +       {
> +       }
> +
> +
> +Variable declarations
> +=====================
> +
> +The order of variables that are declared, should first keep the same types
> +together, but also should be ordered by their length such that the variables
> +are ordered in an "upside-down Christmas tree" fashion where the length gets
> +smaller.
> +
> +       int tracecmd_count_cpus(void)
> +       {
> +               static int once;
> +               char buf[1024];
> +               int cpus = 0;
> +               char *pbuf;
> +               size_t *pn;
> +               FILE *fp;
> +               size_t n;
> +               int r;
> +
> +The above shows that the order is done by length, and in the above example it
> +also shows that "int cpu = 0;" is not grouped next to "int r;". As this is more
> +of a guideline and made to be more aesthetic to the eye of the reader, both the
> +above and is acceptable as below.
> +
> +       int tracecmd_count_cpus(void)
> +       {
> +               static int once;
> +               char buf[1024];
> +               char *pbuf;
> +               size_t *pn;
> +               FILE *fp;
> +               size_t n;
> +               int cpus = 0;
> +               int r;
> +
> +
> +Unless variables are tightly related, it is expected that each variable be on
> +its own line and not grouped by type. That is,
> +
> +               int r, cpus = 0;
> +
> +is to be discouraged, as the two variables are not related to each other.
> +But if you had a bunch of counters:
> +
> +               int i, j, k;
> +
> +That would be fine, as the variables are all related as they are all for the
> +same purpose (arbitrary counters). The same may go with pointers;
> +
> +
> +       char *begin, *end;
> +

One extra paragraph for arranging the members of a struct will be very
useful. I still wonder sometimes how to arrange them so the memory
will be used in a most optimal way.

> +Comments
> +========
> +
> +Comments will use the "/* */" format and the C++ "//" style is discouraged.
> +If a comment is on one line, keep the "/*" and "*/" on the same line:
> +
> +       /* This is a single line comment. */
> +
> +If a comment spans more than one line, then have the "/*" on a separate line
> +before the comment and the "*/" on a separate line at the end of the comment,
> +and each line starts with a "*" where all the "*" line up with each other.
> +
> +       /*
> +        * This is a multi line comment, where all the '*'
> +        * will line up, and the text is on a separate line
> +        * as the start and end markers.
> +        */
> +
> +
> +Function documentation
> +======================
> +
> +All global functions (and especially any APIs) should have a function
> +description in the form of "kernel doc":
> +
> +  https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html
> +
> +The form is:
> +
> +  /**
> +   * function_name() - Brief description of function.
> +   * @arg1: Describe the first argument.
> +   * @arg2: Describe the second argument.
> +   *        One can provide multiple line descriptions
> +   *        for arguments.
> +   *
> +   * A longer description, with more discussion of the function function_name()
> +   * that might be useful to those using or modifying it. Begins with an
> +   * empty comment line, and may include additional embedded empty
> +   * comment lines.
> +   *
> +   * The longer description may have multiple paragraphs.
> +   *
> +   * Context: Describes whether the function can sleep, what locks it takes,
> +   *          releases, or expects to be held. It can extend over multiple
> +   *          lines.
> +   * Return: Describe the return value of function_name.
> +   *
> +   * The return value description can also have multiple paragraphs, and should
> +   * be placed at the end of the comment block.
> +   */
> +
> +General
> +=======
> +
> +As stated, this is a guide and may not be strictly enforced. The goal is to
> +have consistent and readable code. In general, try to have the coding style
> +match the surrounding code.

I agree with Sameer, we should encourage people to use the
checkpatch.pl script from the kernel tree, before submitting the
patch. I find this script very useful, should be mentioned here or in
the CONTRIBUTE file.

[..]


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center