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

2016-10-18 Thread Zhu, Yonghong 
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 ; edk2-devel@lists.01.org
Cc: Gao, Liming ; Zhu, Yonghong 
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 ; edk2-devel@lists.01.org
> Cc: Gao, Liming 
> 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 
> > Cc: Jordan Justen 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Yonghong Zhu 
> > ---
> >  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
> > ^ 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

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

2016-10-18 Thread Jordan Justen 
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 ; edk2-devel@lists.01.org
> Cc: Gao, Liming 
> 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 
> > Cc: Jordan Justen 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Yonghong Zhu 
> > ---
> >  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
> > ^ 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

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

2016-10-17 Thread Jordan Justen 
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 
> Cc: Jordan Justen 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Yonghong Zhu 
> ---
>  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
> ^ 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

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

2016-10-17 Thread Yonghong Zhu 
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

Cc: Liming Gao 
Cc: Jordan Justen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yonghong Zhu 
---
 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 '\t' 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
^ 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