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

[Jordan Justen <jordan.l.justen@intel.com>]

On 2016-10-18 21:40:16, Zhu, Yonghong wrote:
> Hi Jordan,
> 
> If we change regex to r'''DEBUG \s* \( \s* \(\s*. EFI_D_*''', it cannot
> work because we would use mo.group(1) which report IndexError.
> If we change regex to r'''DEBUG \s* \( \s* \(\s*. EFI_D_ ([A-Z_]+)''' ,
> it cannot detect the case "DEBUG ((DEBUG_ERROR | EFI_D_INFO, "xxx", Status));"

Sorry, to not be clear.

I meant r'''DEBUG \s* \( \s* \( \s* EFI_D_ ([A-Z_]+)'''

Here is another option:

r'''
    DEBUG \s* \( \s* \(
    (?: DEBUG_[A-Z_]+ \s* \| \s*)*
    EFI_D_ ([A-Z_]+)
'''

The down side of .* is that it will first match the entire rest of the
line, and then try to match shorter strings. Usually it will end up
trying all of the substrings to the end of the string.

-Jordan

> So how about keep the original regex ?
> 
> Best Regards,
> Zhu Yonghong
> 
> 
> -----Original Message-----
> From: Justen, Jordan L 
> Sent: Wednesday, October 19, 2016 2:32 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 V2] BaseTools/PatchCheck.py: Update to report error for EFI_D_*
> 
> On 2016-10-18 03:16:53, 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=143
> > 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 | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/BaseTools/Scripts/PatchCheck.py 
> > b/BaseTools/Scripts/PatchCheck.py index 05f8f6e..083438c 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -338,10 +338,16 @@ class GitDiffCheck:
> >              lines.append('File: ' + self.hunk_filename)
> >          lines.append('Line: ' + line)
> >  
> >          self.error(*lines)
> >  
> > +    old_debug_re = \
> > +        re.compile(r'''
> > +                       DEBUG \s* \( \s* \( .* EFI_D_ ([A-Z_]+)
>                                               ^^
> 
> I recommend changing .* in the regex to \s*. EFI_D_* is the first parameter. The only thing that might appear is a comment, but that could also appear between the parentheses. This would be an unusual place to put a comment.
> 
> Aside from that,
> 
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> 
> > +                   ''',
> > +                   re.VERBOSE)
> > +
> >      def check_added_line(self, line):
> >          eol = ''
> >          for an_eol in self.line_endings:
> >              if line.endswith(an_eol):
> >                  eol = an_eol
> > @@ -355,10 +361,16 @@ class GitDiffCheck:
> >          if '    ' in line:
> >              self.added_line_error('Tab character used', line)
> >          if len(stripped) < len(line):
> >              self.added_line_error('Trailing whitespace found', line)
> >  
> > +        mo = self.old_debug_re.search(line)
> > +        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)
> > +
> >      split_diff_re = re.compile(r'''
> >                                     (?P<cmd>
> >                                         ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $
> >                                     )
> >                                     (?P<index>
> > --
> > 2.6.1.windows.1
> >