Re: [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_*

["Zhu, Yonghong" <yonghong.zhu@intel.com>]

Thanks Jordan, I sent a V2. I update a little on the re.compile to cover the case that EFI_D_* in the middle of the DEBUG string, and may have multiple space between DEBUG and ( character.

Best Regards,
Zhu Yonghong


-----Original Message-----
From: Justen, Jordan L 
Sent: Tuesday, October 18, 2016 2:43 PM
To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>
Subject: RE: [Patch 3/3] BaseTools: Update PatchCheck to report error for EFI_D_*

On 2016-10-17 19:02:57, Zhu, Yonghong wrote:
> Hi Jordan,
> 
> We request to report error since the DEBUG_* is our recommend coding 
> style and EFI_D_ is obsolete. Looking for DEBUG and EFI_D_ since only 
> look for EFI_D_ may cause some False error be reported. And I agree 
> that user might put EFI_D_ on a separate line, however I scanned our 
> current edk2 code, no such usage. So I'd like to add this support 
> after we meet the real case.
> 

In that case, how about making the regex detect both?

    old_debug_re = \
        re.compile(r'''
                       DEBUG \s? \( \s? \( \s* EFI_D_ ([A-Z_]+)
                   ''',
                   re.VERBOSE)

Then you can use something like:

    mo = self.old_debug_re.search(...)
    if mo is not None:
        self.added_line_error('EFI_D_' + mo.group(1) + ' was used, '
                              'but DEBUG_' + mo.group(1) +
                              ' is now recommended', line)

-Jordan

> 
> -----Original Message-----
> From: Justen, Jordan L
> Sent: Tuesday, October 18, 2016 12:58 AM
> To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [Patch 3/3] BaseTools: Update PatchCheck to report error 
> for EFI_D_*
> 
> On 2016-10-17 01:28:38, Yonghong Zhu wrote:
> > In EDK2, DEBUG_* is recommended to be used instead of EFI_D_*. For 
> > new code, they should use DEBUG_* macro.
> > 
> > Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=145
> >
> 
> Remove blank line. I think you meant 143, not 145.
> 
> Instead of looking for DEBUG and EFI_D_, I think we should just look 
> for EFI_D_. It is possible that the user might put the EFI_D_ on a 
> separate line. What do you think?
> 
> Should we add 'warning' support to the script first, and then flag 
> this as a warning rather than an error? (I don't think this is too 
> important, because getting an error from the script will not currently 
> prevent the patch from being merged.)
> 
> -Jordan
> 
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
> > ---
> >  BaseTools/Scripts/PatchCheck.py | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/BaseTools/Scripts/PatchCheck.py 
> > b/BaseTools/Scripts/PatchCheck.py index 05f8f6e..3ac0769 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -337,10 +337,15 @@ class GitDiffCheck:
> >          if self.hunk_filename is not None:
> >              lines.append('File: ' + self.hunk_filename)
> >          lines.append('Line: ' + line)
> >  
> >          self.error(*lines)
> > +
> > +    DEBUG_macro_re = re.compile(r'''^
> > +                                    \s*DEBUG\s*\(\(
> > +                                ''',
> > +                                re.VERBOSE)
> >  
> >      def check_added_line(self, line):
> >          eol = ''
> >          for an_eol in self.line_endings:
> >              if line.endswith(an_eol):
> > @@ -354,10 +359,13 @@ class GitDiffCheck:
> >                                    line)
> >          if '    ' in line:
> >              self.added_line_error('Tab character used', line)
> >          if len(stripped) < len(line):
> >              self.added_line_error('Trailing whitespace found', 
> > line)
> > +        if self.DEBUG_macro_re.match(line):
> > +            if 'EFI_D_' in line:
> > +                self.added_line_error('EFI_D_* format is used, 
> > + recommend to use DEBUG_* format', line)
> >  
> >      split_diff_re = re.compile(r'''
> >                                     (?P<cmd>
> >                                         ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $
> >                                     )
> > --
> > 2.6.1.windows.1
> >