Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1682772 > unrolled thread

Re: [PATCH 2/2] trace-cmd: replace show_file() -> show_instance_file()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] trace-cmd: replace show_file() ->  show_instance_file() Steven Rostedt <rostedt@goodmis.org> - 2017-07-07 00:30 +0200
    Re: [PATCH 2/2] trace-cmd: replace show_file() -> show_instance_file() Federico Vaga <federico.vaga@vaga.pv.it> - 2017-07-10 02:20 +0200

#1682772 — Re: [PATCH 2/2] trace-cmd: replace show_file() -> show_instance_file()

On Mon,  5 Jun 2017 11:31:18 +0200
Federico Vaga <federico.vaga@vaga.pv.it> wrote:

> show_file(name) and show_instance_file(&top_instance, name) are
> equivalent.
> 
> Remove the show_file() function in order to have a single function for
> this task.

Actually I find nothing wrong with having a helper function like this.
IIRC, show_file() was first, and then show_instance_file() came later.
There's some files that only exist for the top_instance, and I like the
fact that this is annotated that way.

I'm curious to know what the benefit of removing show_file() is?

-- Steve


> 
> Signed-off-by: Federico Vaga <federico.vaga@vaga.pv.it>
> ---
>  trace-list.c     | 21 ++++++---------------
>  trace-local.h    |  2 --
>  trace-show.c     | 18 ++----------------
>  trace-snapshot.c |  2 +-
>  4 files changed, 9 insertions(+), 34 deletions(-)
\

[toc] | [next] | [standalone]

#1683867 — Re: [PATCH 2/2] trace-cmd: replace show_file() -> show_instance_file()

On Friday, July 7, 2017 12:29:35 AM CEST Steven Rostedt wrote:
> On Mon,  5 Jun 2017 11:31:18 +0200
> 
> Federico Vaga <federico.vaga@vaga.pv.it> wrote:
> > show_file(name) and show_instance_file(&top_instance, name) are
> > equivalent.
> > 
> > Remove the show_file() function in order to have a single function for
> > this task.
> 
> Actually I find nothing wrong with having a helper function like this.
> IIRC, show_file() was first, and then show_instance_file() came later.
> There's some files that only exist for the top_instance, and I like the
> fact that this is annotated that way.
> 
> I'm curious to know what the benefit of removing show_file() is?

The show_file(name) and show_instance_file(&top_instance, name) are 
equivalent: they do the same thing. By removing `show_file` the developers are 
forced to use always the same function and being explicit about the instance 
they want to use.

The name `show_file()` is so generic that does not implies automatically that 
we are accessing the top_instance. This is not even clear by reading the 
implementation; people must read the other functions used in `show_file()` to 
understand that their instance scope is always 'top_instance'.

So, in my opinion, it makes the code easier to read and more explicit in what 
is doing without too much effort.

> -- Steve
> 
> > Signed-off-by: Federico Vaga <federico.vaga@vaga.pv.it>
> > ---
> > 
> >  trace-list.c     | 21 ++++++---------------
> >  trace-local.h    |  2 --
> >  trace-show.c     | 18 ++----------------
> >  trace-snapshot.c |  2 +-
> >  4 files changed, 9 insertions(+), 34 deletions(-)
> 
> \


-- 
Federico Vaga
http://www.federicovaga.it/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web