[Spice-devel] [warning reduction 00/11] Eliminating warnings in xf86-video-qxl

[Jeremy White]

> Some general comments:
> 
> - You have this pattern where you assign a string to a global variable
>   and then you assign that variable to a field instead of the string
>   itself. If this really is necessary, I think the string should just be
>   cast instead. I haven't been able to make gcc generate that warning
>   though. Which option did you use?

I spent some time on Google trying to understand the 'correct' way
to fix -Wcast-qual issue (and simple casts did not work, at lest in my attempts).

I didn't especially like the result myself, but it does seem to fit
with what is considered standard practice.  On reflection, I can see
an argument that the resulting code is more correct, given the X headers.

On 09/12/2012 04:11 AM, Christophe Fergeau wrote:
> When I looked at this, this was caused by -Wcast-qual, and I tried to
> address it with
> http://lists.freedesktop.org/archives/spice-devel/2012-May/008998.html
[His patch adds a line to configure.ac to turn off -Wcast-qual]

> 
> - I think -Wshadow is a lost cause. We need to be able to use x1, y1,
>   y2, x2, and x_1 etc. is just too ugly.

y1() is a defined Bessel function; it is a legitimate, if rare, and
highly annoying, name space collision issue.

But I certainly see your point.

I see two ways forward:  we are either aggressive in keeping all warnings
on, and suffering with a few patches we don't like as a result.  Or we
make it a deliberate process to disable warnings we don't like.  Right now,
it looks like that list would include -Wcast-qual and -Wshadow.

I have a preference for the first approach.  To me, if you can avoid adding
your first '-Wno-XXX' stanza to your configure.ac, you can strive for
a simplicity and cleanliness that is valuable.  You also avoid a theoretical
future in which a -Wcast-qual helps catch a legitimate issue, but you
missed it because it's disabled.

However, I will cheerfully shift to the second approach if that is preferred
by you guys.  I just want my compiles to stop masking my
legitimate screw ups <grin>.

> 
> - Some of your commit headlines are too long. These show up in release
>   notes, so please try to keep them below 72 characters.

Yeah, I always make this mistake, and it bugs me that there
is this undocumented rule in git.

In case it helps anyone else, you must make a deliberate effort
in a git commit log to have a separated 'First Line' (e.g. a line
with a full line of white space after it).  That will then
go as the subject of the patch.  If you do not,
then git-format-patch runs it all together on one line and you
end up looking like an idiot.

> 
> - As Alon said, inlined patches are easier to read. Also, if you can
>   point to a git repository, that makes it much easier to merge the
>   patches. We can probably get you a freedesktop.org account if you want
>   one.

Yeah, my apologizes.  I have too many email clients to beat into shape, and
I didn't beat the one at home prior to sending.  I can easily set up a private
fdo tree if it helps to pull rather than git am.

> 
>> The final one does not remove a warning, but documents the related code
>> with a TODO as the warning appears to be correct.
>>
>> You still get a large number of redudant decl warnings from two xorg include
>> files even with this; adding -Wno-redundant-decls to CFLAGS suppresses
>> those.
> 
> It may be worthwhile doing this.

Yah; this would be the third one on our list if we were to start down that
road.

Cheers,

Jeremy