[Bug 670302] Review Request: libbacklight - Linux backlight abstraction library

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=670302

Ralf Corsepius <rc040203 at freenet.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rc040203 at freenet.de

--- Comment #1 from Ralf Corsepius <rc040203 at freenet.de> 2011-01-17 23:27:30 EST ---
Comments:

a) To mjg59 in his role as upstream:

- libbacklight.h lacks extern "C" guards.
=> API is not C++ safe.

- libbacklight.c suffers from implicit decls:

libbacklight.c: In function 'backlight_get':
libbacklight.c:74:2: warning: implicit declaration of function 'read'
libbacklight.c:80:2: warning: implicit declaration of function 'strtol'
libbacklight.c:83:2: warning: implicit declaration of function 'close'
libbacklight.c: In function 'backlight_set_brightness':
libbacklight.c:125:2: warning: implicit declaration of function 'write'
...
=> Code is like to suffer from portability (type-size) issues.
You'll likely want to "#include <unistd.h>"

- Consider to a licensing headers to all source files and a LICENCE/COPYING
file.

- Consider to provide at least a minimum amount of documentation.
ATM, there is no trace of documentation, not even comments in libbacklight.h
describing what these functions are supposed to do.


b) to mjg59 in his role as packager:

- configure.ac applies AM_SILENT_RULES.
=> AM_SILENT_RULES causes build.log to skip essential information (e.g.
compiler calls). I'd recommend upstream to remove AM_SILENT_RULES from
configure.ac. Alternatively, add --disable-silent-rules to %configure

- package applies pkg-config
=> Missing BR: pkg-config
(The fact pkg-config is implicitly being pulled in, is a random coincidence)

- Unnecessary "BR: libtool".
Please remove it, It's not needed.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.