| Message ID | 20130521153341.4880.11812.stgit@hds.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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.
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
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("*")
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(-)