Re: [edk2-devel] [PATCH 1/1] BaseTools: Update PatchCheck.py to check for __FUNCTION__

Re: [edk2-devel] [PATCH 1/1] BaseTools: Update PatchCheck.py to check for __FUNCTION__

[Rebecca Cran]

Could I get some reviews on this please?


-- 

Rebecca Cran


On 4/6/23 7:30 PM, Rebecca Cran wrote:
> New code should use the C99 macro __func__ instead of the pre-Standard
> macro __FUNCTION__. Update PatchCheck.py to reject patches with the
> latter.
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>   BaseTools/Scripts/PatchCheck.py | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 5d17d99a12ef..5f96b05dbcec 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -491,6 +491,10 @@ class GitDiffCheck:
>                                     'but DEBUG_' + mo.group(1) +
>                                     ' is now recommended', line)
>   
> +        if line.find('__FUNCTION__') != -1 and not self.filename.endswith('.py'):
> +            self.added_line_error('__FUNCTION__ was used, but __func__ '
> +                                  'is now recommended', line)
> +
>       split_diff_re = re.compile(r'''
>                                      (?P<cmd>
>                                          ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $

Re: [edk2-devel] [PATCH 1/1] BaseTools: Update PatchCheck.py to check for __FUNCTION__

[Michael D Kinney]

> -----Original Message-----
> From: Rebecca Cran <rebecca@bsdio.com>
> Sent: Tuesday, April 11, 2023 11:06 AM
> To: devel@edk2.groups.io; Gao, Liming <gaoliming@byosoft.com.cn>; Feng, Bob C <bob.c.feng@intel.com>; Chen, Christine
> <yuwei.chen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Update PatchCheck.py to check for __FUNCTION__
> 
> Could I get some reviews on this please?
> 
> 
> --
> 
> Rebecca Cran
> 
> 
> On 4/6/23 7:30 PM, Rebecca Cran wrote:
> > New code should use the C99 macro __func__ instead of the pre-Standard
> > macro __FUNCTION__. Update PatchCheck.py to reject patches with the
> > latter.
> >
> > Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> > ---
> >   BaseTools/Scripts/PatchCheck.py | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> > index 5d17d99a12ef..5f96b05dbcec 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -491,6 +491,10 @@ class GitDiffCheck:
> >                                     'but DEBUG_' + mo.group(1) +
> >                                     ' is now recommended', line)
> >
> > +        if line.find('__FUNCTION__') != -1 and not self.filename.endswith('.py'):

Should use we os.path services to get the file extension?

There are may other file types that this check should be excluded
(e.g. .sh, .gitignore, .gitmodules, .uni).  Should this check only
apply to .c, .h.  What about .C and .H and also the addition of unit
tests that use c++ file extensions?

> > +            self.added_line_error('__FUNCTION__ was used, but __func__ '
> > +                                  'is now recommended', line)
> > +
> >       split_diff_re = re.compile(r'''
> >                                      (?P<cmd>
> >                                          ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $

Re: [edk2-devel] [PATCH 1/1] BaseTools: Update PatchCheck.py to check for __FUNCTION__

[Rebecca Cran]

On 4/11/23 2:21 PM, Michael D Kinney wrote:
> Should use we os.path services to get the file extension?
>
> There are may other file types that this check should be excluded
> (e.g. .sh, .gitignore, .gitmodules, .uni).  Should this check only
> apply to .c, .h.  What about .C and .H and also the addition of unit
> tests that use c++ file extensions?

__func__ is valid in C99 and C++11, so it should apply to .c, .h, .C, 
.H, .cpp, .hpp etc.

I was wondering if we might have shell scripts that generate code too, 
for example OvmfPkg/QemuVideoDxe/VbeShim.sh, in which case we might only 
want to exclude .py which would catch this change to PatchCheck.py.


-- 
Rebecca Cran

Re: [edk2-devel] [PATCH 1/1] BaseTools: Update PatchCheck.py to check for __FUNCTION__

[Michael D Kinney]

I agree C related source files and scripts that generate C code would be included.

Some of BaseTools python scripts generate .c/.h files too, so don't want to exclude all .py.

Perhaps just exclude __file__?

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Rebecca Cran
> Sent: Tuesday, April 11, 2023 2:30 PM
> To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Feng,
> Bob C <bob.c.feng@intel.com>; Chen, Christine <yuwei.chen@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/1] BaseTools: Update PatchCheck.py to check for __FUNCTION__
> 
> On 4/11/23 2:21 PM, Michael D Kinney wrote:
> > Should use we os.path services to get the file extension?
> >
> > There are may other file types that this check should be excluded
> > (e.g. .sh, .gitignore, .gitmodules, .uni).  Should this check only
> > apply to .c, .h.  What about .C and .H and also the addition of unit
> > tests that use c++ file extensions?
> 
> __func__ is valid in C99 and C++11, so it should apply to .c, .h, .C,
> .H, .cpp, .hpp etc.
> 
> I was wondering if we might have shell scripts that generate code too,
> for example OvmfPkg/QemuVideoDxe/VbeShim.sh, in which case we might only
> want to exclude .py which would catch this change to PatchCheck.py.
> 
> 
> --
> Rebecca Cran
> 
> 
> 
> 
>