Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 26 Apr 2021 18:31:02 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Aditya Srivastava <yashsri421@gmail.com>
Cc:     corbet@lwn.net, lukas.bulwahn@gmail.com,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC] scripts: kernel-doc: reduce repeated regex expressions
 into variables
Message-ID: <20210426173102.GO235567@casper.infradead.org>
References: <20210422191839.6119-1-yashsri421@gmail.com>
 <20210423132117.GB235567@casper.infradead.org>
 <6f76ddcb-7076-4c91-9c4c-995002c4cb91@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6f76ddcb-7076-4c91-9c4c-995002c4cb91@gmail.com>
Precedence: bulk

On Sat, Apr 24, 2021 at 05:27:34PM +0530, Aditya Srivastava wrote:
> On 23/4/21 6:51 pm, Matthew Wilcox wrote:
> > On Fri, Apr 23, 2021 at 12:48:39AM +0530, Aditya Srivastava wrote:
> >> +my $pointer_function = qr{([^\(]*\(\*)\s*\)\s*\(([^\)]*)\)};
> > 
> > Is that a pointer-to-function?  Or as people who write C usually call it,
> > a function pointer?  Wouldn't it be better to call it $function_pointer?
> > 
> Will do it.
> 
> >> @@ -1210,8 +1211,14 @@ sub dump_struct($$) {
> >>      my $decl_type;
> >>      my $members;
> >>      my $type = qr{struct|union};
> >> +    my $packed = qr{__packed};
> >> +    my $aligned = qr{__aligned};
> >> +    my $cacheline_aligned_in_smp = qr{____cacheline_aligned_in_smp};
> >> +    my $cacheline_aligned = qr{____cacheline_aligned};
> > 
> > I don't think those four definitions actually simplify anything.
> > 
> >> +    my $attribute = qr{__attribute__\s*\(\([a-z0-9,_\*\s\(\)]*\)\)}i;
> > 
> > ... whereas this one definitely does.
> > 
> >> -	$members =~ s/\s*__attribute__\s*\(\([a-z0-9,_\*\s\(\)]*\)\)/ /gi;
> >> -	$members =~ s/\s*__aligned\s*\([^;]*\)/ /gos;
> >> -	$members =~ s/\s*__packed\s*/ /gos;
> >> +	$members =~ s/\s*$attribute/ /gi;
> >> +	$members =~ s/\s*$aligned\s*\([^;]*\)/ /gos;
> > 
> > Maybe put the \s*\([^;]*\) into $aligned?  Then it becomes a useful
> > abstraction.
> 
> Actually, I had made these variables as they were repeated here and at
> -    my $definition_body =
> qr{\{(.*)\}(?:\s*(?:__packed|__aligned|____cacheline_aligned_in_smp|____cacheline_aligned|__attribute__\s*\(\([a-z0-9,_\s\(\)]*\)\)))*};
> +    my $definition_body =
> qr{\{(.*)\}(?:\s*(?:$packed|$aligned|$cacheline_aligned_in_smp|$cacheline_aligned|$attribute))*};
> 
> So, defining them at a place might help.
> 
> What do you think?

I don't think that seeing $packed is any easier to read than __packed.
Indeed, I think it's harder, because now I have to look up what $packed
is defined as.

Defining a variable, say

	$decorations = qr{__packed|__aligned|____cacheline_aligned_in_smp|____cacheline_aligned|__attribute__\s*\(\([a-z0-9,_\s\(\)]*\)\))}
	(i didn't count brackets to be sure i got that right)

would be helpful because then we could say:

	my $definition_body = qr{\{(.*)\}...$decorations...

and have a fighting chance of understanding what it means.

Now, this other place we use it, we do the =~ operation a number of times.
Is there a way to use the $decorations variable to do the same thing
with a single operation?