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....@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming....@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....@intel.com>
> > Cc: Jordan Justen <jordan.l.jus...@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Yonghong Zhu <yonghong....@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
> > 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to