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.
> 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 > ------------------------------------------------------------------------------ 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
