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

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [Patch 3/3 V2] BaseTools/PatchCheck.py: Update to report error for EFI_D_*
@ 2016-10-18 10:16 Yonghong Zhu
  2016-10-18 18:31 ` Jordan Justen
  0 siblings, 1 reply; 4+ messages in thread
From: Yonghong Zhu @ 2016-10-18 10:16 UTC (permalink / raw)
  To: edk2-devel; +Cc: Liming Gao, Jordan Justen

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_]+)
+                   ''',
+                   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 '\t' 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



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [Patch 3/3 V2] BaseTools/PatchCheck.py: Update to report error for EFI_D_*
  2016-10-18 10:16 [Patch 3/3 V2] BaseTools/PatchCheck.py: Update to report error for EFI_D_* Yonghong Zhu
@ 2016-10-18 18:31 ` Jordan Justen
  2016-10-19  4:40   ` Zhu, Yonghong
  0 siblings, 1 reply; 4+ messages in thread
From: Jordan Justen @ 2016-10-18 18:31 UTC (permalink / raw)
  To: Yonghong Zhu, edk2-devel; +Cc: Liming Gao

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
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Patch 3/3 V2] BaseTools/PatchCheck.py: Update to report error for EFI_D_*
  2016-10-18 18:31 ` Jordan Justen
@ 2016-10-19  4:40   ` Zhu, Yonghong
  2016-10-19  5:29     ` Jordan Justen
  0 siblings, 1 reply; 4+ messages in thread
From: Zhu, Yonghong @ 2016-10-19  4:40 UTC (permalink / raw)
  To: Justen, Jordan L, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zhu, Yonghong

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));"
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
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Patch 3/3 V2] BaseTools/PatchCheck.py: Update to report error for EFI_D_*
  2016-10-19  4:40   ` Zhu, Yonghong
@ 2016-10-19  5:29     ` Jordan Justen
  0 siblings, 0 replies; 4+ messages in thread
From: Jordan Justen @ 2016-10-19  5:29 UTC (permalink / raw)
  To: Zhu, Yonghong, edk2-devel@lists.01.org; +Cc: Gao, Liming, Zhu, Yonghong

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
> > 


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-10-19  5:29 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-18 10:16 [Patch 3/3 V2] BaseTools/PatchCheck.py: Update to report error for EFI_D_* Yonghong Zhu
2016-10-18 18:31 ` Jordan Justen
2016-10-19  4:40   ` Zhu, Yonghong
2016-10-19  5:29     ` Jordan Justen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox