On Wed, 09 Feb 2011 23:48:01 -0500 Christopher Michael <[email protected]> wrote:
> On 02/09/2011 06:49 PM, [email protected] wrote: > > I really hate to nit-pick, but it's still not correct :/ > > > > _backlight = XInternAtom (_ecore_x_disp, RANDR_PROPERTY_BACKLIGHT, True); > > > > should be: > > > > _backlight = XInternAtom(_ecore_x_disp, RANDR_PROPERTY_BACKLIGHT, True); > > > > IE: We don't use spaces between function and function parameters. > > > > Too lazy to rewrite all the lines by hand when I read them from the > > xbacklight code. That's why. I corrected my mistakes. > > > > This is done in a few places in this patch, and yet others in this patch > > are ok: > > > > XRRPropertyInfo *info = XRRQueryOutputProperty(_ecore_x_disp, output, > > _backlight); > > > > So we have a 'mixed' bit of formatting here :/ > > > > > > Also: > > if (actual_type != XA_INTEGER || nitems != 1 || actual_format != 32) > > > > Should be done like: > > if ((actual_type != XA_INTEGER) || (nitems != 1) || (actual_format != 32)) > > > > agreed, much better this way and it suppress a warning if the compiler is > > picky (I tried with -Wall -Wextra and still no warnings for this one). > > > > And again here: > > if (info->range&& info->num_values == 2) > > > > Should be: > > if ((info->range)&& (info->num_values == 2)) > > > > disagree here :-). No need to put the first operand between parenthesis. > > Personally, I find that putting parenthesis around expressions that do not > > contain test is an useless and overcharging notation while it is better > > when expressions contain tests such as the second operand. I did modify it > > following your advise. > > > Well, you are of course free to disagree :) BUT the fact remains, in EFL > code it is preferable to use the parenthesis. > Actually we got into a big flame war about this a while back and agreed that in cases where it's something like: if (X && (!Y)) then it's perfectly fine to not use parens around the X. > > Bad: > > if(!_ecore_x_randr_output_validate(root, output)) > > Good: > > if (!_ecore_x_randr_output_validate(root, output)) > > > > > > Please declare your variables at the beginning of the function .. NOT > > inline like these: > > > > + /* get the ressources */ > > + XRRScreenResources *resources = _ecore_x_randr_get_screen_resources > > (_ecore_x_disp, root); > > > > + int o; > > + for (o = 0; o< resources->noutput; o++) > > > > Again, here, I modified the code accordingly but I find much easier to > > declare dummy variables where there are used only. The example you gave > > would be much better like that. > > > LOL, well again, you are free to disagree. BUT in EFL, it's done with > variables @ the top of the function (99% of the time). > > > for(int o=0;...;...) > > > > You do not have to worry about the variable afterward and it avoids > > annoying warning such as unused variables from gcc. > Well, another way to avoid those annoying warnings is to just NOT > declare variables you don't plan on using ;) > > Of course it means that the c89 standard is not respected (I think it > is coming from the c99 standard as well as inline functions that are > used all over the place). And it has been supported by most compilers > for ages ;-). > > > > Functionally, the patch looks sane. I have not tested it tho. If you > > could address the above issues, I'd be more than happy to apply locally > > and test it out. > > > > Thanks, please try it out. If you want to test it out, just compile this > > small program. It setups the backlight to 100 percent (you have to modify > > the backlight before of course). Another thing, your driver needs to > > support it. intel does, nouveau (most probably) but I do not know about Ati. > > > No worries :) Will check it out in the morning. (nvidia driver here) ;) > > dh > > > cheers too. > > > > Mathieu > > On every other case, however, devilhorns is 100% correct. -- Mike Blumenkrantz Zentific: NULL pointer dereferences now 50% off! ------------------------------------------------------------------------------ The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE: Pinpoint memory and threading errors before they happen. Find and fix more than 250 security defects in the development cycle. Locate bottlenecks in serial and parallel code that limit performance. http://p.sf.net/sfu/intel-dev2devfeb _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
