On Tue, Jan 25, 2011 at 3:23 PM, Siarhei Siamashka <[email protected]> wrote: > On Monday 24 January 2011 11:51:39 Rolland Dudemaine wrote: >> Hi, >> >> Granted, the patch was somewhat too big and contained many things. >> >> Attached the patch splitted in five parts : >> - *fence_prototype.diff resolves the incorrect fence_malloc prototype. >> It also correctly protects onglobal_msg and on_alarm behind >> HAVE_SIGACTION and HAVE_ALARM, since it's otherwise useless and >> unreferenced. If you still want to split these further into separate >> patches, feel free to do so, this is trivial enough to do. > > Pushed this fix (without 'on_alarm' so far) to pixman git master. > >> - *max_vx.diff corrects the initialization of max_vx. Contrary to what >> you mention, the variable is declared in a local block, so the scope of >> the variable is restricted to that block, regardless of whether it is >> part of a macro or not. So the compiler is correct saying the value may >> be used before being initialized. Also, INT32_MAX is a safe value, >> because pixman-compiler.h defines it. INT32_MAX is the right value to >> use since max_vx is a clamping value passed on to a callee. 0 would just >> clamp down to 0, which would be plainly incorrect. > > Pushed this one too. Even though I think that your analysis is wrong, I don't > want to waste time arguing about the choice of the constant ;)
I agree with Siarhei, max_vx is used in scanline_func if and only if the repeat mode is NORMAL, so it should not actually matter. Anyway, INT32_MAX is obviously a safe value so your patch is ok for me. Since any value is fine in that initialization, the problem with pixman_fixed_t becoming a different type in future is moot as well. I think that initializing to 0 would be ok as well, but I doubt that it's an optimization worth the additional readability cost. > >> - *return_values.diff defines return values where missing. I added it as >> a general return value, you may want to move that inside the switch >> statement right above, this is really a coding style issue. >> >> - *set_but_not_used.diff removes useless variable declarations. This can >> only result in more efficient code, as these variables where sometimes >> assigned, but their values were never used. >> >> - *correct_dimmy_PIXMAN_types.diff is the most complex part of the >> patches. My compiler was producing a number of warnings due to incorrect >> uses of int instead of the correct corresponding pixman_*_t type. Also, >> it was complaining (rightfully) that some values were passed as >> pixman_format_code_t that were not defined in that enum. The best >> resolution I could come up with was to logically move PIXMAN_null, >> PIXMAN_any and the related dummy pixmap types to the enum, so that it is >> possible to use them as argument where a pixman_format_code_t argument >> is required. (Note : the attached patch contains one more correction for >> blitters-test.c that I did not correct previously) > > The biggest complaint against 'correct_dimmy_PIXMAN_types.diff' is that it > moves the definitions of PIXMAN_solid, PIXMAN_pixbuf, PIXMAN_rpixbuf, > PIXMAN_unknown, PIXMAN_any, PIXMAN_OP_any to the public header 'pixman.h'. > And these are intended for internal use only and not supposed to be part of > external API. Are there any good alternative fixes for the problem? We can add a cast to pixman_op_t or pixman_format_code_t in the #defines. Another solution (which I don't like) would be to protect the new enum values with an "#ifdef PIXMAN_USE_INTERNAL_API", just like in pixman.h for PIXMAN_N_OPERATORS. > > PIXMAN_null is a bit different, because it indeed should by returned from > 'pixman_image_get_format' function instead of 0. > > Another problem is that it is an extra burden on me (or any other committer) > to come up with commit message texts for these warnings cleanup patches, > which are even not bugfixes. It would have been much easier to deal with > the patches generated by 'git format-patch' > > I have put some of the other your changes (but not all yet) into this branch: > http://cgit.freedesktop.org/~siamashka/pixman/log/?h=warnings-fixes > If everybody is ok with these changes and commit messages, I'll push them > to pixman git repository shortly. Then we can look at what is still remaining. The branch looks good to me. Andrea > > -- > Best regards, > Siarhei Siamashka > _______________________________________________ > Pixman mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/pixman > _______________________________________________ Pixman mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/pixman
