Patchwork [RFC,v3,02/11] Fix errors and warnings while compiling with c++ compilier

login
register
mail settings
Submitter Tomoki Sekiyama
Date May 21, 2013, 3:33 p.m.
Message ID <20130521153341.4880.11812.stgit@hds.com>
Download mbox | patch
Permalink /patch/245322/
State New
Headers show

Comments

Tomoki Sekiyama - May 21, 2013, 3:33 p.m.
Add C++ keywords to avoid errors in compiling with c++ compiler.
This also renames class member of PciDeviceInfo to q_class.

Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
---
 hmp.c           |    2 +-
 hw/pci/pci.c    |    2 +-
 scripts/qapi.py |    9 ++++++++-
 3 files changed, 10 insertions(+), 3 deletions(-)
Stefan Hajnoczi - May 23, 2013, 12:12 p.m.
On Tue, May 21, 2013 at 11:33:41AM -0400, Tomoki Sekiyama wrote:
> Add C++ keywords to avoid errors in compiling with c++ compiler.
> This also renames class member of PciDeviceInfo to q_class.
> 
> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
> ---
>  hmp.c           |    2 +-
>  hw/pci/pci.c    |    2 +-
>  scripts/qapi.py |    9 ++++++++-
>  3 files changed, 10 insertions(+), 3 deletions(-)

Please also extend scripts/checkpatch.pl.  Otherwise it is very likely
that C++ keywords will be introduced again in the future.  Most people
will not build the VSS code and therefore checkpatch.pl needs to ensure
that patches with C++ keywords will not be accepted.

Stefan
Tomoki Sekiyama - May 23, 2013, 6:34 p.m.
On 5/23/13 8:12 , "Stefan Hajnoczi" <stefanha@gmail.com> wrote:

>On Tue, May 21, 2013 at 11:33:41AM -0400, Tomoki Sekiyama wrote:
>> Add C++ keywords to avoid errors in compiling with c++ compiler.
>> This also renames class member of PciDeviceInfo to q_class.
>> 
>> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
>> ---
>>  hmp.c           |    2 +-
>>  hw/pci/pci.c    |    2 +-
>>  scripts/qapi.py |    9 ++++++++-
>>  3 files changed, 10 insertions(+), 3 deletions(-)
>
>Please also extend scripts/checkpatch.pl.  Otherwise it is very likely
>that C++ keywords will be introduced again in the future.  Most people
>will not build the VSS code and therefore checkpatch.pl needs to ensure
>that patches with C++ keywords will not be accepted.
>
>Stefan

I think it would be difficult to identify problematic C++ keywords usage
from patches because headers can legally contain C++ keywords and
checkpatch.pl doesn't know where it should be used.
Do you have any good ideas?

And, at least for QAPI, scripts/qapi.py automatically detects the
keywords to add q_ prefix for them (like `q_class' in this patch).

Thanks,
Tomoki Sekiyama
Stefan Hajnoczi - May 24, 2013, 8:52 a.m.
On Thu, May 23, 2013 at 06:34:43PM +0000, Tomoki Sekiyama wrote:
> On 5/23/13 8:12 , "Stefan Hajnoczi" <stefanha@gmail.com> wrote:
> 
> >On Tue, May 21, 2013 at 11:33:41AM -0400, Tomoki Sekiyama wrote:
> >> Add C++ keywords to avoid errors in compiling with c++ compiler.
> >> This also renames class member of PciDeviceInfo to q_class.
> >> 
> >> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
> >> ---
> >>  hmp.c           |    2 +-
> >>  hw/pci/pci.c    |    2 +-
> >>  scripts/qapi.py |    9 ++++++++-
> >>  3 files changed, 10 insertions(+), 3 deletions(-)
> >
> >Please also extend scripts/checkpatch.pl.  Otherwise it is very likely
> >that C++ keywords will be introduced again in the future.  Most people
> >will not build the VSS code and therefore checkpatch.pl needs to ensure
> >that patches with C++ keywords will not be accepted.
> >
> >Stefan
> 
> I think it would be difficult to identify problematic C++ keywords usage
> from patches because headers can legally contain C++ keywords and
> checkpatch.pl doesn't know where it should be used.
> Do you have any good ideas?

We can ignore false warnings for 0.1% of patches (the ones that touch
VSS C++ code).  But for the remaining 99.9% of patches it's worth
guarding against VSS bitrot.

Remember not many people will compile it and therefore they won't notice
when they break it.  I really think it's worth putting some effort in
now so VSS doesn't periodically break.

checkpatch.pl is a hacky sort of C parser.  It already checks for a
bunch of similar things and it knows about comments, macros, and
strings.  It does not perform #include expansion, so there is no risk of
including system headers that have C++ code.

Stefan
Laszlo Ersek - May 24, 2013, 1:01 p.m.
On 05/21/13 17:33, Tomoki Sekiyama wrote:

> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index afc5f32..b174acb 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -156,9 +156,16 @@ def c_var(name, protect=True):
>      # GCC http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/C-Extensions.html
>      # excluding _.*
>      gcc_words = set(['asm', 'typeof'])
> +    # C++ ISO/IEC 14882:2003 2.11
> +    cpp_words = set(['bool', 'catch', 'class', 'const_cast', 'delete',
> +                     'dynamic_cast', 'explicit', 'false', 'friend', 'mutable',
> +                     'namespace', 'new', 'operator', 'private', 'protected',
> +                     'public', 'reinterpret_cast', 'static_cast', 'template',
> +                     'this', 'throw', 'true', 'try', 'typeid', 'typename',
> +                     'using', 'virtual', 'wchar_t'])
>      # namespace pollution:
>      polluted_words = set(['unix'])
> -    if protect and (name in c89_words | c99_words | c11_words | gcc_words | polluted_words):
> +    if protect and (name in c89_words | c99_words | c11_words | gcc_words | cpp_words | polluted_words):
>          return "q_" + name
>      return name.replace('-', '_').lstrip("*")

Since you're respinning anyway, perhaps consider adding these lovely
"alternative representations" from just one paragraph below (they are
reserved and "shall not be used otherwise" than the operators they stand
for):

and	bitand	compl	not_eq	or_eq	xor_eq
and_eq	bitor	not	or	xor

although probably noone would use these as identifiers or otherwise...
So just mentioning it for completeness.

Laszlo
Tomoki Sekiyama - May 24, 2013, 2:20 p.m.
On 5/24/13 4:52 , "Stefan Hajnoczi" <stefanha@gmail.com> wrote:

>On Thu, May 23, 2013 at 06:34:43PM +0000, Tomoki Sekiyama wrote:
>> On 5/23/13 8:12 , "Stefan Hajnoczi" <stefanha@gmail.com> wrote:
>> 
>> >On Tue, May 21, 2013 at 11:33:41AM -0400, Tomoki Sekiyama wrote:
>> >> Add C++ keywords to avoid errors in compiling with c++ compiler.
>> >> This also renames class member of PciDeviceInfo to q_class.
>> >> 
>> >> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
>> >> ---
>> >>  hmp.c           |    2 +-
>> >>  hw/pci/pci.c    |    2 +-
>> >>  scripts/qapi.py |    9 ++++++++-
>> >>  3 files changed, 10 insertions(+), 3 deletions(-)
>> >
>> >Please also extend scripts/checkpatch.pl.  Otherwise it is very likely
>> >that C++ keywords will be introduced again in the future.  Most people
>> >will not build the VSS code and therefore checkpatch.pl needs to ensure
>> >that patches with C++ keywords will not be accepted.
>> >
>> >Stefan
>> 
>> I think it would be difficult to identify problematic C++ keywords usage
>> from patches because headers can legally contain C++ keywords and
>> checkpatch.pl doesn't know where it should be used.
>> Do you have any good ideas?
>
>We can ignore false warnings for 0.1% of patches (the ones that touch
>VSS C++ code).  But for the remaining 99.9% of patches it's worth
>guarding against VSS bitrot.
>
>Remember not many people will compile it and therefore they won't notice
>when they break it.  I really think it's worth putting some effort in
>now so VSS doesn't periodically break.
>
>checkpatch.pl is a hacky sort of C parser.  It already checks for a
>bunch of similar things and it knows about comments, macros, and
>strings.  It does not perform #include expansion, so there is no risk of
>including system headers that have C++ code.
>
>Stefan

Thanks for your comment.

I'm still wondering because it actually causes a lot false positives
(not just 0.1%...) even for the patches not touching VSS.

For example, keyword 'class' is used in qdev-monitor.c, qom/object.c,
and a lot of files in hw/*/*.c and include/{hw,qom}/*.h, but
they have nothing to do with qemu-ga. Qemu-ga is just a part of whole
qemu source code, so I don't want to warn around the other parts.

Thanks,
Tomoki Sekiyama
Tomoki Sekiyama - May 24, 2013, 2:33 p.m.
On 5/24/13 9:01 , "Laszlo Ersek" <lersek@redhat.com> wrote:

>On 05/21/13 17:33, Tomoki Sekiyama wrote:
>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index afc5f32..b174acb 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -156,9 +156,16 @@ def c_var(name, protect=True):
>>      # GCC http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/C-Extensions.html
>>      # excluding _.*
>>      gcc_words = set(['asm', 'typeof'])
>> +    # C++ ISO/IEC 14882:2003 2.11
>> +    cpp_words = set(['bool', 'catch', 'class', 'const_cast', 'delete',
>> +                     'dynamic_cast', 'explicit', 'false', 'friend',
>>'mutable',
>> +                     'namespace', 'new', 'operator', 'private',
>>'protected',
>> +                     'public', 'reinterpret_cast', 'static_cast',
>>'template',
>> +                     'this', 'throw', 'true', 'try', 'typeid',
>>'typename',
>> +                     'using', 'virtual', 'wchar_t'])
>>      # namespace pollution:
>>      polluted_words = set(['unix'])
>> -    if protect and (name in c89_words | c99_words | c11_words |
>>gcc_words | polluted_words):
>> +    if protect and (name in c89_words | c99_words | c11_words |
>>gcc_words | cpp_words | polluted_words):
>>          return "q_" + name
>>      return name.replace('-', '_').lstrip("*")
>
>Since you're respinning anyway, perhaps consider adding these lovely
>"alternative representations" from just one paragraph below (they are
>reserved and "shall not be used otherwise" than the operators they stand
>for):
>
>and	bitand	compl	not_eq	or_eq	xor_eq
>and_eq	bitor	not	or	xor
>
>although probably noone would use these as identifiers or otherwise...
>So just mentioning it for completeness.
>
>Laszlo

OK, I will try adding these keywords in next submit.

Thanks,
Tomoki Sekiyama
Markus Armbruster - May 24, 2013, 3:25 p.m.
Tomoki Sekiyama <tomoki.sekiyama@hds.com> writes:

> On 5/24/13 4:52 , "Stefan Hajnoczi" <stefanha@gmail.com> wrote:
>
>>On Thu, May 23, 2013 at 06:34:43PM +0000, Tomoki Sekiyama wrote:
>>> On 5/23/13 8:12 , "Stefan Hajnoczi" <stefanha@gmail.com> wrote:
>>> 
>>> >On Tue, May 21, 2013 at 11:33:41AM -0400, Tomoki Sekiyama wrote:
>>> >> Add C++ keywords to avoid errors in compiling with c++ compiler.
>>> >> This also renames class member of PciDeviceInfo to q_class.
>>> >> 
>>> >> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
>>> >> ---
>>> >>  hmp.c           |    2 +-
>>> >>  hw/pci/pci.c    |    2 +-
>>> >>  scripts/qapi.py |    9 ++++++++-
>>> >>  3 files changed, 10 insertions(+), 3 deletions(-)
>>> >
>>> >Please also extend scripts/checkpatch.pl.  Otherwise it is very likely
>>> >that C++ keywords will be introduced again in the future.  Most people
>>> >will not build the VSS code and therefore checkpatch.pl needs to ensure
>>> >that patches with C++ keywords will not be accepted.
>>> >
>>> >Stefan
>>> 
>>> I think it would be difficult to identify problematic C++ keywords usage
>>> from patches because headers can legally contain C++ keywords and
>>> checkpatch.pl doesn't know where it should be used.
>>> Do you have any good ideas?
>>
>>We can ignore false warnings for 0.1% of patches (the ones that touch
>>VSS C++ code).  But for the remaining 99.9% of patches it's worth
>>guarding against VSS bitrot.
>>
>>Remember not many people will compile it and therefore they won't notice
>>when they break it.  I really think it's worth putting some effort in
>>now so VSS doesn't periodically break.
>>
>>checkpatch.pl is a hacky sort of C parser.  It already checks for a
>>bunch of similar things and it knows about comments, macros, and
>>strings.  It does not perform #include expansion, so there is no risk of
>>including system headers that have C++ code.
>>
>>Stefan
>
> Thanks for your comment.
>
> I'm still wondering because it actually causes a lot false positives
> (not just 0.1%...) even for the patches not touching VSS.
>
> For example, keyword 'class' is used in qdev-monitor.c, qom/object.c,
> and a lot of files in hw/*/*.c and include/{hw,qom}/*.h, but
> they have nothing to do with qemu-ga. Qemu-ga is just a part of whole
> qemu source code, so I don't want to warn around the other parts.

And I appreciate that.  Purging some other language's keywords feels
like pointless churn to me.
Stefan Hajnoczi - May 27, 2013, 11:43 a.m.
On Fri, May 24, 2013 at 05:25:21PM +0200, Markus Armbruster wrote:
> Tomoki Sekiyama <tomoki.sekiyama@hds.com> writes:
> 
> > On 5/24/13 4:52 , "Stefan Hajnoczi" <stefanha@gmail.com> wrote:
> >
> >>On Thu, May 23, 2013 at 06:34:43PM +0000, Tomoki Sekiyama wrote:
> >>> On 5/23/13 8:12 , "Stefan Hajnoczi" <stefanha@gmail.com> wrote:
> >>> 
> >>> >On Tue, May 21, 2013 at 11:33:41AM -0400, Tomoki Sekiyama wrote:
> >>> >> Add C++ keywords to avoid errors in compiling with c++ compiler.
> >>> >> This also renames class member of PciDeviceInfo to q_class.
> >>> >> 
> >>> >> Signed-off-by: Tomoki Sekiyama <tomoki.sekiyama@hds.com>
> >>> >> ---
> >>> >>  hmp.c           |    2 +-
> >>> >>  hw/pci/pci.c    |    2 +-
> >>> >>  scripts/qapi.py |    9 ++++++++-
> >>> >>  3 files changed, 10 insertions(+), 3 deletions(-)
> >>> >
> >>> >Please also extend scripts/checkpatch.pl.  Otherwise it is very likely
> >>> >that C++ keywords will be introduced again in the future.  Most people
> >>> >will not build the VSS code and therefore checkpatch.pl needs to ensure
> >>> >that patches with C++ keywords will not be accepted.
> >>> >
> >>> >Stefan
> >>> 
> >>> I think it would be difficult to identify problematic C++ keywords usage
> >>> from patches because headers can legally contain C++ keywords and
> >>> checkpatch.pl doesn't know where it should be used.
> >>> Do you have any good ideas?
> >>
> >>We can ignore false warnings for 0.1% of patches (the ones that touch
> >>VSS C++ code).  But for the remaining 99.9% of patches it's worth
> >>guarding against VSS bitrot.
> >>
> >>Remember not many people will compile it and therefore they won't notice
> >>when they break it.  I really think it's worth putting some effort in
> >>now so VSS doesn't periodically break.
> >>
> >>checkpatch.pl is a hacky sort of C parser.  It already checks for a
> >>bunch of similar things and it knows about comments, macros, and
> >>strings.  It does not perform #include expansion, so there is no risk of
> >>including system headers that have C++ code.
> >>
> >>Stefan
> >
> > Thanks for your comment.
> >
> > I'm still wondering because it actually causes a lot false positives
> > (not just 0.1%...) even for the patches not touching VSS.
> >
> > For example, keyword 'class' is used in qdev-monitor.c, qom/object.c,
> > and a lot of files in hw/*/*.c and include/{hw,qom}/*.h, but
> > they have nothing to do with qemu-ga. Qemu-ga is just a part of whole
> > qemu source code, so I don't want to warn around the other parts.
> 
> And I appreciate that.  Purging some other language's keywords feels
> like pointless churn to me.

I see what you guys are saying now.  You want to protect only qemu-ga.
I was proposing protecting the entire codebase so we never get into a
situation where changing a header breaks qemu-ga.

It's not that hard to use "klass" instead of "class", but if it's
unpopular then we'll just have to live with VSS breakage.

Stefan

Patch

diff --git a/hmp.c b/hmp.c
index 4fb76ec..cb0ed3b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -484,7 +484,7 @@  static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev)
     if (dev->class_info.has_desc) {
         monitor_printf(mon, "%s", dev->class_info.desc);
     } else {
-        monitor_printf(mon, "Class %04" PRId64, dev->class_info.class);
+        monitor_printf(mon, "Class %04" PRId64, dev->class_info.q_class);
     }
 
     monitor_printf(mon, ": PCI device %04" PRIx64 ":%04" PRIx64 "\n",
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d5257ed..a3eaf47 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1460,7 +1460,7 @@  static PciDeviceInfo *qmp_query_pci_device(PCIDevice *dev, PCIBus *bus,
     info->function = PCI_FUNC(dev->devfn);
 
     class = pci_get_word(dev->config + PCI_CLASS_DEVICE);
-    info->class_info.class = class;
+    info->class_info.q_class = class;
     desc = get_class_desc(class);
     if (desc->desc) {
         info->class_info.has_desc = true;
diff --git a/scripts/qapi.py b/scripts/qapi.py
index afc5f32..b174acb 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -156,9 +156,16 @@  def c_var(name, protect=True):
     # GCC http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/C-Extensions.html
     # excluding _.*
     gcc_words = set(['asm', 'typeof'])
+    # C++ ISO/IEC 14882:2003 2.11
+    cpp_words = set(['bool', 'catch', 'class', 'const_cast', 'delete',
+                     'dynamic_cast', 'explicit', 'false', 'friend', 'mutable',
+                     'namespace', 'new', 'operator', 'private', 'protected',
+                     'public', 'reinterpret_cast', 'static_cast', 'template',
+                     'this', 'throw', 'true', 'try', 'typeid', 'typename',
+                     'using', 'virtual', 'wchar_t'])
     # namespace pollution:
     polluted_words = set(['unix'])
-    if protect and (name in c89_words | c99_words | c11_words | gcc_words | polluted_words):
+    if protect and (name in c89_words | c99_words | c11_words | gcc_words | cpp_words | polluted_words):
         return "q_" + name
     return name.replace('-', '_').lstrip("*")