GNOME Bugzilla – Bug 674876
Fix some clang warnings (and a potential leak)
Last modified: 2012-05-02 14:20:09 UTC
Clang warns about a lot of things. Most of them aren't fixable, because some of the things that clang warns about, gcc either warns when removed (like potentially unset variables that really aren't), or are there for consistency with other pieces of code. But of the ones that are, this patch set fixes them!
Created attachment 212887 [details] [review] texture-tower: Remove potential leak The expression in here does not match the one where we malloc the data.
Created attachment 212888 [details] [review] texture-tower: Remove undefined behavior Petty, but clang's static analysis complains about this happening, where it could take both the FALSE and TRUE branches of the dest_texture_height < source_texture_height condition to end up in a place where we crash. Since underflow of unsigned integers is specified, rather than undefined, this should correct possible semantics.
Created attachment 212889 [details] [review] window-group: Remove an unnecessary assignment Clang warns about this.
Created attachment 212890 [details] [review] default plugin: Remove start method that does nothing All animations use the constants directly, so this is just declaring a bunch of local variables and then doing nothing with it. Another clang warning.
Review of attachment 212887 [details] [review]: Actually, there's no leak, because dest_height is always <= dest_texture_heigh, so if dest_texture_height < source_texture_height, dest_height < source_height - so what there actually is instead is a potential g_free (NULL), which is harmless. But yes, it's a bug, and sharp eyes for Clang.
Review of attachment 212888 [details] [review]: Well, for starters, mixing guint and unsigned int in the same block isn't OK. But also I dont' understand the commit message. (Underflow is a floating-point phenomena)
Review of attachment 212887 [details] [review]: Well, it wasn't entirely clang-detected. While adding 'unsigned's to the next patch, it warned about a comparison between signed and unsigned integers.
Review of attachment 212888 [details] [review]: All in all, this didn't stop the warning. I wish I knew why clang's static analysis tool is failing. A developer in #llvm seems to think it's an llvm bug.
Review of attachment 212889 [details] [review]: Looks good
Review of attachment 212890 [details] [review]: OK
(In reply to comment #7) > Review of attachment 212887 [details] [review]: > > Well, it wasn't entirely clang-detected. While adding 'unsigned's to the next > patch, it warned about a comparison between signed and unsigned integers. What warning was it giving with the original code?
LLVM reported bug: http://llvm.org/bugs/show_bug.cgi?id=12675
Created attachment 212905 [details] [review] boxes: Remove an unnecessary assignment Two more easy clang fixes.
Created attachment 212906 [details] [review] window: Remove setting of an unused variable Well, technically it's used, but only in one block, so let's just make it scoped to that block.
Attachment 212887 [details] pushed as cd7a74f - texture-tower: Remove potential leak Attachment 212889 [details] pushed as 25d3432 - window-group: Remove an unnecessary assignment Attachment 212890 [details] pushed as ebf8c46 - default plugin: Remove start method that does nothing
Review of attachment 212905 [details] [review]: Sure
Review of attachment 212906 [details] [review]: Sure
Created attachment 213250 [details] [review] window-actor: Another simple clang warning fix If we explicitly check for a NULL pointer, clang will assume that the pointer may be NULL at some point. We clearly rely on the pointer being non-NULL earlier, so fix this guy up.
Attachment 212905 [details] pushed as a24c512 - boxes: Remove an unnecessary assignment Attachment 212906 [details] pushed as a78fec7 - window: Remove setting of an unused variable
Review of attachment 213250 [details] [review]: Yep
Attachment 213250 [details] pushed as 878b101 - window-actor: Another simple clang warning fix