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

Groups > linux.kernel > #1700356 > 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-31 21:40 +0200
    Re: [PATCH 2/2] trace-cmd: replace show_file() ->  show_instance_file() Federico Vaga <federico.vaga@vaga.pv.it> - 2017-08-01 01:30 +0200

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

On Mon, 10 Jul 2017 02:15:35 +0200
Federico Vaga <federico.vaga@vaga.pv.it> wrote:

> 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.
> 

Just an FYI. You'll find lots of these types of helper functions in the
Linux Kernel. As I'm a Linux Kernel developer, I prefer them ;-)

-- Steve

[toc] | [next] | [standalone]

#1700502

On Mon, Jul 31, 2017 at 03:35:39PM -0400, Steven Rostedt wrote:
> On Mon, 10 Jul 2017 02:15:35 +0200
> Federico Vaga <federico.vaga@vaga.pv.it> wrote:
> 
>> 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.
>> 
> 
> Just an FYI. You'll find lots of these types of helper functions in the
> Linux Kernel. As I'm a Linux Kernel developer, I prefer them ;-)

We are kind of artists: there is always a bit of personal taste 
(preferences)
in what we do; that justify our presence instead of an AI (for the time 
being) :P

As I said in the other email, I tend to be verbose when something does 
not have
an unique interpretation at first sight.

If this is your preference, I will not say anything more and I will not 
send a V2
for this patch.


Federico Vaga

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web