Hi,
continuation of review on Melange:
On Sat, Dec 3, 2011 at 2:46 AM, ym9...@gmail.com <ym9...@gmail.com> wrote:
> -// av_log(NULL,AV_LOG_ERROR, "%X\n", av_lfg_get(&state));
> - x+=av_lfg_get(&state);
> +// av_log(NULL,AV_LOG_ERROR, "%X\n", av_lfg_get(&state));
>
Space between "NULL," and "AV_LOG_ERROR".
> -static int use_color=-1;
> +static int use_color =- 1;
>
"= -1", not "=- 1".
> - use_color = (con != INVALID_HANDLE_VALUE) && !getenv("NO_COLOR")
> && !getenv("AV_LOG_FORCE_NOCOLOR");
> + use_color = (con != INVALID_HANDLE_VALUE) && !getenv("NO_COLOR")
> + && !getenv("AV_LOG_FORCE_NOCOLOR");
>
"&&" should go on previous line.
> - use_color= !getenv("NO_COLOR") && !getenv("AV_LOG_FORCE_NOCOLOR")
> &&
> + use_color = !getenv("NO_COLOR") &&
> !getenv("AV_LOG_FORCE_NOCOLOR") &&
> (getenv("TERM") && isatty(2) || getenv("AV_LOG_FORCE_COLOR"));
> #else
> - use_color= getenv("AV_LOG_FORCE_COLOR") && !getenv("NO_COLOR") &&
> !getenv("AV_LOG_FORCE_NOCOLOR");
> + use_color = getenv("AV_LOG_FORCE_COLOR") && !getenv("NO_COLOR")
> + && !getenv("AV_LOG_FORCE_NOCOLOR");
>
Same for both here.
> - AVClass* avc= ptr ? *(AVClass**)ptr : NULL;
> - if(level>av_log_level)
> + AVClass* avc = ptr ? *(AVClass**)ptr : NULL;
>
* (AVClass **) ptr.
> + if (level>av_log_level)
>
Spaces around ">".
> - AVClass** parent= *(AVClass***)(((uint8_t*)ptr) +
> avc->parent_log_context_offset);
> - if(parent && *parent){
> - snprintf(line, sizeof(line), "[%s @ %p] ",
> (*parent)->item_name(parent), parent);
> + AVClass** parent = *(AVClass***)(((uint8_t*)ptr)
>
* (AVClass ***) (((uint8_t *) ptr).
+ + avc->parent_log_context_offset);
>
+ goes on previous line.
> + if (parent && *parent) {
> + snprintf(line, sizeof(line), "[%s @ %p] ",
> + (*parent)->item_name(parent), parent);
>
Please align ( with first character after ( on pevious line, i.e. 9 more
spaces.
> - snprintf(line + strlen(line), sizeof(line) - strlen(line), "[%s @
> %p] ", avc->item_name(ptr), ptr);
> + snprintf(line + strlen(line), sizeof(line) - strlen(line), "[%s @
> %p] ",
> + avc->item_name(ptr), ptr);
>
One more space, so the 'a' is vertically aligned with the 'l' in the line
above.
> - if(!is_atty) is_atty= isatty(2) ? 1 : -1;
> + if (!is_atty) is_atty= isatty(2) ? 1 : -1;
>
Add a space left of the '=', and put the if(..) on a separate line from the
is_atty = ...
> - if(print_prefix && (flags & AV_LOG_SKIP_REPEATED) && !strncmp(line,
> prev, sizeof line)){
> + if (print_prefix && (flags & AV_LOG_SKIP_REPEATED)
> + && !strncmp(line, prev, sizeof line)) {
>
&& goes on the line above.
-static void (*av_log_callback)(void*, int, const char*, va_list) =
> av_log_default_callback;
> +static void (*av_log_callback)(void*, int, const char*, va_list)
> + = av_log_default_callback;
>
'=' goes on the line above.
- AVClass* avc= avcl ? *(AVClass**)avcl : NULL;
> + AVClass* avc = avcl ? *(AVClass**)avcl : NULL;
>
* (AVClass **) avcl
- if(avc && avc->version >= (50<<16 | 15<<8 | 2) &&
> avc->log_level_offset_offset && level>=AV_LOG_FATAL)
> + if (avc && avc->version >= (50<<16 | 15<<8 | 2) &&
> avc->log_level_offset_offset
> + && level>=AV_LOG_FATAL)
>
&& goes on the line above.
> -void av_vlog(void* avcl, int level, const char *fmt, va_list vl)
> -{
> +void av_vlog(void* avcl, int level, const char *fmt, va_list vl) {
>
No, { gets its own line.
> -int av_log_get_level(void)
> -{
> +int av_log_get_level(void) {
>
[..]
> -void av_log_set_level(int level)
> -{
> +void av_log_set_level(int level) {
>
[..]
> -void av_log_set_flags(int arg)
> -{
> - flags= arg;
> +void av_log_set_flags(int arg) {
>
[..]
> -void av_log_set_callback(void (*callback)(void*, int, const char*,
> va_list))
> -{
> +void av_log_set_callback(void (*callback)(void*, int, const char*,
> va_list)) {
>
Same for all of the above.
> diff --git a/libavutil/random_seed.c b/libavutil/random_seed.c
>
[..]
> -static int read_random(uint32_t *dst, const char *file)
> -{
> +static int read_random(uint32_t *dst, const char *file) {
>
Same.
> -static uint32_t get_generic_seed(void)
> -{
> - clock_t last_t=0;
> - int bits=0;
> - uint64_t random=0;
> +static uint32_t get_generic_seed(void) {
>
Same.
- for(i=0;bits<64;i++){
> - clock_t t= clock();
> - if(last_t && fabs(t-last_t)>s || t==(clock_t)-1){
> - if(i<10000 && s<(1<<24)){
> - s+=s;
> - i=t=0;
> - }else{
> - random= 2*random + (i&1);
> + for (i = 0;bits < 64; i++) {
>
Space between ';' and "bits".
+ clock_t t = clock();
> + if (last_t && fabs(t-last_t)>s || t == (clock_t) - 1) {
>
Spaces around '-' and '>'.
+ if (i < 10000 && s < (1<<24)) {
>
Spaces around "<<".
> + random = 2 * random + (i&1);
>
Spaces around '&'.
> -uint32_t av_get_random_seed(void)
> -{
> +uint32_t av_get_random_seed(void) {
>
See above, { gets its own line.
> diff --git a/libavutil/rational.c b/libavutil/rational.c
>
[..]
> -int av_reduce(int *dst_num, int *dst_den, int64_t num, int64_t den,
> int64_t max){
> - AVRational a0={0,1}, a1={1,0};
> - int sign= (num<0) ^ (den<0);
> - int64_t gcd= av_gcd(FFABS(num), FFABS(den));
> -
> - if(gcd){
> - num = FFABS(num)/gcd;
> - den = FFABS(den)/gcd;
> +int av_reduce (int *dst_num, int *dst_den,
> + int64_t num, int64_t den, int64_t max) {
>
{ gets its own line, and the second line should vertically align with the
first function argument in the first line, so "int64_t num" should
vertically align with "int *dst_num".
> + if (num <= max && den <= max) {
> + a1 = (AVRational){ num, den };
>
Space between ')' and '{'.
> - while(den){
> - uint64_t x = num / den;
> - int64_t next_den= num - den*x;
> - int64_t a2n= x*a1.num + a0.num;
> - int64_t a2d= x*a1.den + a0.den;
> + while (den) {
> + uint64_t x = num / den;
> + int64_t next_den = num - den * x;
> + int64_t a2n = x * a1.num + a0.num;
> + int64_t a2d = x * a1.den + a0.den;
>
You could (optionally, I don't really care) vertically align the '=' of the
bottom two lines with the first two lines also. But not important.
> - if (den*(2*x*a1.den + a0.den) > num*a1.den)
> - a1 = (AVRational){x*a1.num + a0.num, x*a1.den + a0.den};
> + if (den*(2 *x *a1.den + a0.den) > num *a1.den)
> + a1 = (AVRational){x *a1.num + a0.num, x *a1.den + a0.den};
>
Spaces after *, and space between ')' and '{'.
> - a0= a1;
> - a1= (AVRational){a2n, a2d};
> - num= den;
> - den= next_den;
> + a0 = a1;
> + a1 = (AVRational){ a2n, a2d };
>
Space between ')' and '{'.
> -AVRational av_mul_q(AVRational b, AVRational c){
> - av_reduce(&b.num, &b.den, b.num * (int64_t)c.num, b.den *
> (int64_t)c.den, INT_MAX);
> +AVRational av_mul_q(AVRational b, AVRational c) {
>
{ gets its own line.
> + av_reduce(&b.num, &b.den, b.num *
> + (int64_t)c.num, b.den * (int64_t)c.den, INT_MAX);
>
This is wrong. First, if at all possible you should split lines so
individual arguments stay on a single line, e.g. 'b_num * (int64_t) c.num'
could be a line of its own, but you should keep half of it on a line with
other stuff and then put the other half on another line. Second, indenting
of the second line should vertically align with the first character after
the '(' in the first line.
> -AVRational av_div_q(AVRational b, AVRational c){
> - return av_mul_q(b, (AVRational){c.den, c.num});
> +AVRational av_div_q(AVRational b, AVRational c) {
>
{ on its own line.
> + return av_mul_q(b, (AVRational){ c.den, c.num });
>
Spaces between ')' and '{'.
- av_reduce(&b.num, &b.den, b.num * (int64_t)c.den + c.num *
> (int64_t)b.den, b.den * (int64_t)c.den, INT_MAX);
> +AVRational av_add_q(AVRational b, AVRational c) {
> + av_reduce(&b.num, &b.den, b.num * (int64_t)c.den + c.num *
> (int64_t)b.den,
> + b.den * (int64_t)c.den, INT_MAX);
> return b;
> }
>
> -AVRational av_sub_q(AVRational b, AVRational c){
> - return av_add_q(b, (AVRational){-c.num, c.den});
> +AVRational av_sub_q(AVRational b, AVRational c) {
> + return av_add_q(b, (AVRational){ -c.num, c.den });
>
See earlier comments for both of these.
> -AVRational av_d2q(double d, int max){
> +AVRational av_d2q(double d, int max) {
>
{ own line.
> - return (AVRational){0,0};
> + return (AVRational){ 0,0 };
>
Space between ')' and '{'.
> - return (AVRational){ d<0 ? -1:1, 0 };
> + return (AVRational){ d<0 ? -1 : 1, 0 };
>
Space between ')' and '{'.
> -int av_nearer_q(AVRational q, AVRational q1, AVRational q2)
> -{
> +int av_nearer_q(AVRational q, AVRational q1, AVRational q2) {
>
{ own line.
> -int av_find_nearest_q_idx(AVRational q, const AVRational* q_list)
> -{
> +int av_find_nearest_q_idx(AVRational q, const AVRational* q_list) {
>
Same.
-int main(void)
> -{
> +int main(void) {
>
{ own line.
> + for (a.num = -2; a.num <= 2; a.num++) {
> + for (a.den = -2; a.den <= 2; a.den++) {
> + for (b.num = -2; b.num <= 2; b.num++) {
> + for (b.den = -2; b.den <=2 ; b.den++) {
>
Space should go before, not after the '2'.
> + int c = av_cmp_q(a,b);
> + double d = av_q2d(a) == av_q2d(b) ? 0 :
> + (av_q2d(a) - av_q2d(b));
>
This is misindented.
> + if (d > 0) d = 1;
> + else if (d < 0) d = -1;
> + else if (d != d) d = INT_MIN;
>
You could add some spaces to the first line (between 'd = ..' and 'if
(..)') so that the "d = " part is vertically aligned for each of these
lines.
+ if (c != d) av_log(0, AV_LOG_ERROR, "%d/%d %d/%d, %d
> %f\n",
> + a.num, a.den, b.num, b.den, c,d);
>
Mis-indented.
> -void *av_tree_find(const AVTreeNode *t, void *key, int (*cmp)(void *key,
> const void *b), void *next[2]){
> - if(t){
> - unsigned int v= cmp(key, t->elem);
> - if(v){
> - if(next) next[v>>31]= t->elem;
> +void *av_tree_find(const AVTreeNode *t, void *key, int (*cmp)(void *key,
> + const void *b), void *next[2]) {
>
Vertically align const on both lines, and { own line.
> + if (t) {
> + unsigned int v = cmp(key, t->elem);
> + if (v) {
> + if (next) next[v>>31] = t->elem;
>
Spaces around ">>".
> -void *av_tree_insert(AVTreeNode **tp, void *key, int (*cmp)(void *key,
> const void *b), AVTreeNode **next){
> - AVTreeNode *t= *tp;
> - if(t){
> - unsigned int v= cmp(t->elem, key);
> +void *av_tree_insert(AVTreeNode **tp, void *key, int (*cmp)(void *key,
> + const void *b), AVTreeNode **next) {
>
See before.
> + else if (t->child[0] || t->child[1]){
>
Space between ')' and '{'.
> + ret = av_tree_insert(&t->child[v>>31], key, cmp, next);
>
Spaces around '>>'.
> + int i = (v>>31) ^ !!*next;
>
Same.
> + if (!(t->state&1)) {
>
Spaces around '&'.
> if((*child)->state*2 == -t->state)
> rotate(child, i^1);
>
Spaces around * and ^.
> + if ((*child)->state * 2 == -t->state) {
> + *tp = (*child)->child[i^1];
> + (*child)->child[i^1] = (*tp)->child[i];
> + (*tp)->child[i] = *child;
> + *child = (*tp)->child[i^1];
> + (*tp)->child[i^1] = t;
>
For all: spaces around ^.
> + *tp = *child;
> + *child = (*child)->child[i^1];
> + (*tp)->child[i^1] = t;
>
Spaces around ^.
> + else t->state>> = 1;
>
Space before ">>", and remove the space between ">>" and "=".
> -void av_tree_destroy(AVTreeNode *t){
> - if(t){
> +void av_tree_destroy(AVTreeNode *t) {
>
{ own line.
> -void av_tree_enumerate(AVTreeNode *t, void *opaque, int (*cmp)(void
> *opaque, void *elem), int (*enu)(void *opaque, void *elem)){
> - if(t){
> - int v= cmp ? cmp(opaque, t->elem) : 0;
> - if(v>=0) av_tree_enumerate(t->child[0], opaque, cmp, enu);
> - if(v==0) enu(opaque, t->elem);
> - if(v<=0) av_tree_enumerate(t->child[1], opaque, cmp, enu);
> +void av_tree_enumerate(AVTreeNode *t, void *opaque, int (*cmp)(void
> *opaque,
> + void *elem), int (*enu)(void *opaque, void *elem)) {
>
See comments before.
> + if (t) {
> + int v = cmp ? cmp(opaque, t->elem) : 0;
> + if (v >= 0) av_tree_enumerate(t->child[0], opaque, cmp, enu);
> + if (v == 0) enu(opaque, t->elem);
> + if (v<=0) av_tree_enumerate(t->child[1], opaque, cmp, enu);
>
Missing spaces in if statement on last line.
> -static void print(AVTreeNode *t, int depth){
> +static void print(AVTreeNode *t, int depth) {
>
{ own line.
> -static int cmp(void *a, const void *b){
> - return (uint8_t*)a-(const uint8_t*)b;
> +static int cmp(void *a, const void *b) {
>
{ own line.
> + return (uint8_t*)a - (const uint8_t*)b;
>
(uint8_t *) a - (const uint8_t *) b;
> -int main(void){
> +int main (void) {
>
{ own line.
> + av_tree_insert(&root, (void*)(j + 1), cmp, &node2);
> + k = av_tree_find(root, (void*)(j + 1), cmp, NULL);
>
Space between "(void *)" and "(j + 1)", also add space between void and *.
Ronald
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
