Re: [PATCH v12 5/9] trace-cmd: Refactored few functions in trace-record.c

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 28 Aug 2019 11:57:42 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> From: Tzvetomir Stoyanov <tstoyanov@vmware.com>

Patch looks good. Just need some fixes in the change log.

Subject: "trace-cmd: Refactored some functions in trace-record.c"

> 
> In order to reuse the code inside the trace-cmd application, few

 "a few"

> functions from trace-record.c are refactored:
>   - make_instances() and tracecmd_remove_instances() are splited.

 "are split"

> New ones are created: tracecmd_make_instance() and tracecmd_remove_instance(),

 "Following functions were added to export static functionality:
    tracecmd_make_instance()
    tracecmd_remove_instance()"


> which are visible outside the trace-record.c

 "outside of trace-record.c"

>   - Following functions are made non-static: tracecmd_init_instance()
> get_instance_dir(), write_instance_file(), write_tracing_on(),
> tracecmd_set_clock()

Make the above more a visible list.

    - The following functions are made non-static:
         tracecmd_init_instance()
         get_instance_dir()
         write_instance_file()
         write_tracing_on()
         tracecmd_set_clock()

And don't be afraid to add empty lines between these bullet points. It
makes it easier to read. Whitespace can be your friend ;-)

>   - New function is implemented: tracecmd_local_cpu_count(), an internal
> API to get local_cpu_count.

Turn the above into:

    - Created a new function:
        tracecmd_local_cpu_count() - an internal API to get the
                                     local_cpu_count.


Note, if you do bullet lists, please keep everything aligned, and space
out the bullet list.

Instead of:

>>>>
  - Following functions are made non-static: tracecmd_init_instance()
get_instance_dir(), write_instance_file(), write_tracing_on(),
tracecmd_set_clock()
  - New function is implemented: tracecmd_local_cpu_count(), an internal
API to get local_cpu_count.
<<<<

Have:

>>>>
  - Following functions are made non-static: tracecmd_init_instance()
    get_instance_dir(), write_instance_file(), write_tracing_on(),
    tracecmd_set_clock()

  - New function is implemented: tracecmd_local_cpu_count(), an internal
    API to get local_cpu_count.
<<<<

Much easier to read.

Proper spacing is critical for change logs. It makes things stand out
much easier when scanning a "git log" looking for something specific.
When there's a lot of text bound together, it's much harder to find
something unless you know exactly what to search for via a search
string.

Thanks!

-- Steve


> 
> Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> ---
>  tracecmd/include/trace-local.h |  9 ++++
>  tracecmd/trace-record.c        | 88 +++++++++++++++++++---------------
>  2 files changed, 59 insertions(+), 38 deletions(-)
>