Re: [Qemu-devel] [RFC PATCH v3 02/11] Fix errors and warnings while compiling with c++ compilier

[Stefan Hajnoczi]

On Fri, May 24, 2013 at 05:25:21PM +0200, Markus Armbruster wrote:
> Tomoki Sekiyama <address@hidden> writes:
> 
> > On 5/24/13 4:52 , "Stefan Hajnoczi" <address@hidden> wrote:
> >
> >>On Thu, May 23, 2013 at 06:34:43PM +0000, Tomoki Sekiyama wrote:
> >>> On 5/23/13 8:12 , "Stefan Hajnoczi" <address@hidden> 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 <address@hidden>
> >>> >> ---
> >>> >>  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