> 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel