| Message ID | 1397728449-8263-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
Hi, I'd prefer that my patch is applied as is, and you are welcome to apply doxygen corrections on top of that. I think we did agree that each programmer uses new lines as see appropriate for code readability. In my opinion, you are removing new lines too aggressively and make code less readable (at least on some places). Could Linaro get you an external display, if laptop resolution is the issue here? -Petri > -----Original Message----- > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov > Sent: Thursday, April 17, 2014 12:54 PM > To: lng-odp@lists.linaro.org > Subject: [lng-odp] [PATCH] scheduler patch code style clean up > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > > Hello Petri, please review this clean up and merge to your patch. > Changes: > > 1. Doxygen warnings fix. > 2. Removed not needed white spaces from timer_test examples which > reduced > generated documentation and code by 1 page. > > > include/odp_schedule.h | 2 +- > platform/linux-generic/source/odp_schedule.c | 1 - > test/timer/odp_timer_test.c | 30 ++------------------ > -------- > 3 files changed, 3 insertions(+), 30 deletions(-) > > diff --git a/include/odp_schedule.h b/include/odp_schedule.h > index 80862c2..8087021 100644 > --- a/include/odp_schedule.h > +++ b/include/odp_schedule.h > @@ -33,7 +33,7 @@ extern "C" { > * > * Converts nanoseconds to wait values for other schedule functions. > * > - * @param time_ns > + * @param ns Nanoseconds > * > * @return Value for the wait parameter in schedule functions > */ > diff --git a/platform/linux-generic/source/odp_schedule.c > b/platform/linux-generic/source/odp_schedule.c > index 12f192b..eb4d9ee 100644 > --- a/platform/linux-generic/source/odp_schedule.c > +++ b/platform/linux-generic/source/odp_schedule.c > @@ -338,7 +338,6 @@ static int schedule_loop(odp_queue_t *out_queue, > uint64_t wait, > > while (1) { > ret = schedule(out_queue, out_buf, max_num, max_deq); > - > if (ret) > break; > > diff --git a/test/timer/odp_timer_test.c b/test/timer/odp_timer_test.c > index af080be..337d80c 100644 > --- a/test/timer/odp_timer_test.c > +++ b/test/timer/odp_timer_test.c > @@ -7,7 +7,7 @@ > /** > * @file > * > - * @example odp_example.c ODP example application > + * @example odp_timer_test.c ODP timer example application > */ > > #include <string.h> > @@ -22,7 +22,6 @@ > /* GNU lib C */ > #include <getopt.h> > > - > #define MAX_WORKERS 32 /**< Max worker threads */ > #define MSG_POOL_SIZE (4*1024*1024) /**< Message pool size */ > > @@ -40,14 +39,13 @@ typedef struct { > int core_count; /**< Core count*/ > } test_args_t; > > - > /** @private Barrier for test synchronisation */ > static odp_barrier_t test_barrier; > > /** @private Timer handle*/ > static odp_timer_t test_timer; > > - > +/** @private test timeout */ > static void test_timeouts(int thr) > { > uint64_t tick; > @@ -58,14 +56,10 @@ static void test_timeouts(int thr) > ODP_DBG(" [%i] test_timeouts\n", thr); > > queue = odp_queue_lookup("timer_queue"); > - > tick = odp_timer_current_tick(test_timer); > - > tick += 100; > - > odp_timer_absolute_tmo(test_timer, tick, > queue, ODP_BUFFER_INVALID); > - > ODP_DBG(" [%i] current tick %"PRIu64"\n", thr, tick); > > while (1) { > @@ -73,18 +67,14 @@ static void test_timeouts(int thr) > > /* TODO: read tick from tmo metadata */ > tick = odp_timer_current_tick(test_timer); > - > ODP_DBG(" [%i] timeout, tick %"PRIu64"\n", thr, tick); > - > odp_buffer_free(buf); > > num--; > - > if (num == 0) > break; > > tick += 100; > - > odp_timer_absolute_tmo(test_timer, tick, > queue, ODP_BUFFER_INVALID); > } > @@ -93,7 +83,6 @@ static void test_timeouts(int thr) > odp_schedule_release_atomic(); > } > > - > /** > * @internal Worker thread > * > @@ -122,7 +111,6 @@ static void *run_thread(void *arg) > * Find the buffer pool > */ > msg_pool = odp_buffer_pool_lookup("msg_pool"); > - > if (msg_pool == ODP_BUFFER_POOL_INVALID) { > ODP_ERR(" [%i] msg_pool not found\n", thr); > return NULL; > @@ -132,13 +120,11 @@ static void *run_thread(void *arg) > > test_timeouts(thr); > > - > printf("Thread %i exits\n", thr); > fflush(NULL); > return arg; > } > > - > /** > * @internal Print help > */ > @@ -151,7 +137,6 @@ static void print_usage(void) > printf("\n\n"); > } > > - > /** > * @internal Parse arguments > * > @@ -172,7 +157,6 @@ static void parse_args(int argc, char *argv[], > test_args_t *args) > > while (1) { > opt = getopt_long(argc, argv, "+c:h", longopts, > &long_index); > - > if (opt == -1) > break; /* No more options */ > > @@ -192,7 +176,6 @@ static void parse_args(int argc, char *argv[], > test_args_t *args) > } > } > > - > /** > * Test main function > */ > @@ -229,15 +212,12 @@ int main(int argc, char *argv[]) > printf("CPU freq (hz): %"PRIu64"\n", odp_sys_cpu_hz()); > printf("Cache line size: %i\n", odp_sys_cache_line_size()); > printf("Max core count: %i\n", odp_sys_core_count()); > - > printf("\n"); > > /* A worker thread per core */ > num_workers = odp_sys_core_count(); > - > if (args.core_count) > num_workers = args.core_count; > - > /* force to max core count */ > if (num_workers > MAX_WORKERS) > num_workers = MAX_WORKERS; > @@ -249,7 +229,6 @@ int main(int argc, char *argv[]) > * Start mapping thread from core #1 > */ > first_core = 1; > - > if (odp_sys_core_count() == 1) > first_core = 0; > > @@ -271,7 +250,6 @@ int main(int argc, char *argv[]) > pool = odp_buffer_pool_create("msg_pool", pool_base, > MSG_POOL_SIZE, > sizeof(test_message_t), > ODP_CACHE_LINE_SIZE, ODP_BUFFER_TYPE_RAW); > - > if (pool == ODP_BUFFER_POOL_INVALID) { > ODP_ERR("Pool create failed.\n"); > return -1; > @@ -286,7 +264,6 @@ int main(int argc, char *argv[]) > param.sched.group = ODP_SCHED_GROUP_DEFAULT; > > queue = odp_queue_create("timer_queue", ODP_QUEUE_TYPE_SCHED, > ¶m); > - > if (queue == ODP_QUEUE_INVALID) { > ODP_ERR("Timer queue create failed.\n"); > return -1; > @@ -294,8 +271,6 @@ int main(int argc, char *argv[]) > > test_timer = odp_timer_create("test_timer", pool, > 1000000, 1000000, 1000000000000); > - > - > odp_shm_print_all(); > > printf("CPU freq %"PRIu64" hz\n", odp_sys_cpu_hz()); > @@ -329,6 +304,5 @@ int main(int argc, char *argv[]) > odp_linux_pthread_join(thread_tbl, num_workers); > > printf("ODP timer test complete\n\n"); > - > return 0; > } > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 17 April 2014 12:59, Savolainen, Petri (NSN - FI/Espoo) < petri.savolainen@nsn.com> wrote: > Hi, > > I'd prefer that my patch is applied as is, and you are welcome to apply > doxygen corrections on top of that. The doxygen fixes should go into your patch Petri I think. > I think we did agree that each programmer uses new lines as see > appropriate for code readability. In my opinion, you are removing new lines > too aggressively and make code less readable (at least on some places). > Could Linaro get you an external display, if laptop resolution is the issue > here? > We had this discussion last week or the week before that, so can we please stop having discussions about whitespace, if it passes checkpatch and git am then we're fine right (or use scripts/odp_check which formats whitespaces). Cheers, > > -Petri > > > > > -----Original Message----- > > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp- > > bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov > > Sent: Thursday, April 17, 2014 12:54 PM > > To: lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCH] scheduler patch code style clean up > > > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > > --- > > > > Hello Petri, please review this clean up and merge to your patch. > > Changes: > > > > 1. Doxygen warnings fix. > > 2. Removed not needed white spaces from timer_test examples which > > reduced > > generated documentation and code by 1 page. > > > > > > include/odp_schedule.h | 2 +- > > platform/linux-generic/source/odp_schedule.c | 1 - > > test/timer/odp_timer_test.c | 30 ++------------------ > > -------- > > 3 files changed, 3 insertions(+), 30 deletions(-) > > > > diff --git a/include/odp_schedule.h b/include/odp_schedule.h > > index 80862c2..8087021 100644 > > --- a/include/odp_schedule.h > > +++ b/include/odp_schedule.h > > @@ -33,7 +33,7 @@ extern "C" { > > * > > * Converts nanoseconds to wait values for other schedule functions. > > * > > - * @param time_ns > > + * @param ns Nanoseconds > > * > > * @return Value for the wait parameter in schedule functions > > */ > > diff --git a/platform/linux-generic/source/odp_schedule.c > > b/platform/linux-generic/source/odp_schedule.c > > index 12f192b..eb4d9ee 100644 > > --- a/platform/linux-generic/source/odp_schedule.c > > +++ b/platform/linux-generic/source/odp_schedule.c > > @@ -338,7 +338,6 @@ static int schedule_loop(odp_queue_t *out_queue, > > uint64_t wait, > > > > while (1) { > > ret = schedule(out_queue, out_buf, max_num, max_deq); > > - > > if (ret) > > break; > > > > diff --git a/test/timer/odp_timer_test.c b/test/timer/odp_timer_test.c > > index af080be..337d80c 100644 > > --- a/test/timer/odp_timer_test.c > > +++ b/test/timer/odp_timer_test.c > > @@ -7,7 +7,7 @@ > > /** > > * @file > > * > > - * @example odp_example.c ODP example application > > + * @example odp_timer_test.c ODP timer example application > > */ > > > > #include <string.h> > > @@ -22,7 +22,6 @@ > > /* GNU lib C */ > > #include <getopt.h> > > > > - > > #define MAX_WORKERS 32 /**< Max worker threads */ > > #define MSG_POOL_SIZE (4*1024*1024) /**< Message pool size */ > > > > @@ -40,14 +39,13 @@ typedef struct { > > int core_count; /**< Core count*/ > > } test_args_t; > > > > - > > /** @private Barrier for test synchronisation */ > > static odp_barrier_t test_barrier; > > > > /** @private Timer handle*/ > > static odp_timer_t test_timer; > > > > - > > +/** @private test timeout */ > > static void test_timeouts(int thr) > > { > > uint64_t tick; > > @@ -58,14 +56,10 @@ static void test_timeouts(int thr) > > ODP_DBG(" [%i] test_timeouts\n", thr); > > > > queue = odp_queue_lookup("timer_queue"); > > - > > tick = odp_timer_current_tick(test_timer); > > - > > tick += 100; > > - > > odp_timer_absolute_tmo(test_timer, tick, > > queue, ODP_BUFFER_INVALID); > > - > > ODP_DBG(" [%i] current tick %"PRIu64"\n", thr, tick); > > > > while (1) { > > @@ -73,18 +67,14 @@ static void test_timeouts(int thr) > > > > /* TODO: read tick from tmo metadata */ > > tick = odp_timer_current_tick(test_timer); > > - > > ODP_DBG(" [%i] timeout, tick %"PRIu64"\n", thr, tick); > > - > > odp_buffer_free(buf); > > > > num--; > > - > > if (num == 0) > > break; > > > > tick += 100; > > - > > odp_timer_absolute_tmo(test_timer, tick, > > queue, ODP_BUFFER_INVALID); > > } > > @@ -93,7 +83,6 @@ static void test_timeouts(int thr) > > odp_schedule_release_atomic(); > > } > > > > - > > /** > > * @internal Worker thread > > * > > @@ -122,7 +111,6 @@ static void *run_thread(void *arg) > > * Find the buffer pool > > */ > > msg_pool = odp_buffer_pool_lookup("msg_pool"); > > - > > if (msg_pool == ODP_BUFFER_POOL_INVALID) { > > ODP_ERR(" [%i] msg_pool not found\n", thr); > > return NULL; > > @@ -132,13 +120,11 @@ static void *run_thread(void *arg) > > > > test_timeouts(thr); > > > > - > > printf("Thread %i exits\n", thr); > > fflush(NULL); > > return arg; > > } > > > > - > > /** > > * @internal Print help > > */ > > @@ -151,7 +137,6 @@ static void print_usage(void) > > printf("\n\n"); > > } > > > > - > > /** > > * @internal Parse arguments > > * > > @@ -172,7 +157,6 @@ static void parse_args(int argc, char *argv[], > > test_args_t *args) > > > > while (1) { > > opt = getopt_long(argc, argv, "+c:h", longopts, > > &long_index); > > - > > if (opt == -1) > > break; /* No more options */ > > > > @@ -192,7 +176,6 @@ static void parse_args(int argc, char *argv[], > > test_args_t *args) > > } > > } > > > > - > > /** > > * Test main function > > */ > > @@ -229,15 +212,12 @@ int main(int argc, char *argv[]) > > printf("CPU freq (hz): %"PRIu64"\n", odp_sys_cpu_hz()); > > printf("Cache line size: %i\n", odp_sys_cache_line_size()); > > printf("Max core count: %i\n", odp_sys_core_count()); > > - > > printf("\n"); > > > > /* A worker thread per core */ > > num_workers = odp_sys_core_count(); > > - > > if (args.core_count) > > num_workers = args.core_count; > > - > > /* force to max core count */ > > if (num_workers > MAX_WORKERS) > > num_workers = MAX_WORKERS; > > @@ -249,7 +229,6 @@ int main(int argc, char *argv[]) > > * Start mapping thread from core #1 > > */ > > first_core = 1; > > - > > if (odp_sys_core_count() == 1) > > first_core = 0; > > > > @@ -271,7 +250,6 @@ int main(int argc, char *argv[]) > > pool = odp_buffer_pool_create("msg_pool", pool_base, > > MSG_POOL_SIZE, > > sizeof(test_message_t), > > ODP_CACHE_LINE_SIZE, > ODP_BUFFER_TYPE_RAW); > > - > > if (pool == ODP_BUFFER_POOL_INVALID) { > > ODP_ERR("Pool create failed.\n"); > > return -1; > > @@ -286,7 +264,6 @@ int main(int argc, char *argv[]) > > param.sched.group = ODP_SCHED_GROUP_DEFAULT; > > > > queue = odp_queue_create("timer_queue", ODP_QUEUE_TYPE_SCHED, > > ¶m); > > - > > if (queue == ODP_QUEUE_INVALID) { > > ODP_ERR("Timer queue create failed.\n"); > > return -1; > > @@ -294,8 +271,6 @@ int main(int argc, char *argv[]) > > > > test_timer = odp_timer_create("test_timer", pool, > > 1000000, 1000000, 1000000000000); > > - > > - > > odp_shm_print_all(); > > > > printf("CPU freq %"PRIu64" hz\n", odp_sys_cpu_hz()); > > @@ -329,6 +304,5 @@ int main(int argc, char *argv[]) > > odp_linux_pthread_join(thread_tbl, num_workers); > > > > printf("ODP timer test complete\n\n"); > > - > > return 0; > > } > > -- > > 1.8.5.1.163.gd7aced9 > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/include/odp_schedule.h b/include/odp_schedule.h index 80862c2..8087021 100644 --- a/include/odp_schedule.h +++ b/include/odp_schedule.h @@ -33,7 +33,7 @@ extern "C" { * * Converts nanoseconds to wait values for other schedule functions. * - * @param time_ns + * @param ns Nanoseconds * * @return Value for the wait parameter in schedule functions */ diff --git a/platform/linux-generic/source/odp_schedule.c b/platform/linux-generic/source/odp_schedule.c index 12f192b..eb4d9ee 100644 --- a/platform/linux-generic/source/odp_schedule.c +++ b/platform/linux-generic/source/odp_schedule.c @@ -338,7 +338,6 @@ static int schedule_loop(odp_queue_t *out_queue, uint64_t wait, while (1) { ret = schedule(out_queue, out_buf, max_num, max_deq); - if (ret) break; diff --git a/test/timer/odp_timer_test.c b/test/timer/odp_timer_test.c index af080be..337d80c 100644 --- a/test/timer/odp_timer_test.c +++ b/test/timer/odp_timer_test.c @@ -7,7 +7,7 @@ /** * @file * - * @example odp_example.c ODP example application + * @example odp_timer_test.c ODP timer example application */ #include <string.h> @@ -22,7 +22,6 @@ /* GNU lib C */ #include <getopt.h> - #define MAX_WORKERS 32 /**< Max worker threads */ #define MSG_POOL_SIZE (4*1024*1024) /**< Message pool size */ @@ -40,14 +39,13 @@ typedef struct { int core_count; /**< Core count*/ } test_args_t; - /** @private Barrier for test synchronisation */ static odp_barrier_t test_barrier; /** @private Timer handle*/ static odp_timer_t test_timer; - +/** @private test timeout */ static void test_timeouts(int thr) { uint64_t tick; @@ -58,14 +56,10 @@ static void test_timeouts(int thr) ODP_DBG(" [%i] test_timeouts\n", thr); queue = odp_queue_lookup("timer_queue"); - tick = odp_timer_current_tick(test_timer); - tick += 100; - odp_timer_absolute_tmo(test_timer, tick, queue, ODP_BUFFER_INVALID); - ODP_DBG(" [%i] current tick %"PRIu64"\n", thr, tick); while (1) { @@ -73,18 +67,14 @@ static void test_timeouts(int thr) /* TODO: read tick from tmo metadata */ tick = odp_timer_current_tick(test_timer); - ODP_DBG(" [%i] timeout, tick %"PRIu64"\n", thr, tick); - odp_buffer_free(buf); num--; - if (num == 0) break; tick += 100; - odp_timer_absolute_tmo(test_timer, tick, queue, ODP_BUFFER_INVALID); } @@ -93,7 +83,6 @@ static void test_timeouts(int thr) odp_schedule_release_atomic(); } - /** * @internal Worker thread * @@ -122,7 +111,6 @@ static void *run_thread(void *arg) * Find the buffer pool */ msg_pool = odp_buffer_pool_lookup("msg_pool"); - if (msg_pool == ODP_BUFFER_POOL_INVALID) { ODP_ERR(" [%i] msg_pool not found\n", thr); return NULL; @@ -132,13 +120,11 @@ static void *run_thread(void *arg) test_timeouts(thr); - printf("Thread %i exits\n", thr); fflush(NULL); return arg; } - /** * @internal Print help */ @@ -151,7 +137,6 @@ static void print_usage(void) printf("\n\n"); } - /** * @internal Parse arguments * @@ -172,7 +157,6 @@ static void parse_args(int argc, char *argv[], test_args_t *args) while (1) { opt = getopt_long(argc, argv, "+c:h", longopts, &long_index); - if (opt == -1) break; /* No more options */ @@ -192,7 +176,6 @@ static void parse_args(int argc, char *argv[], test_args_t *args) } } - /** * Test main function */ @@ -229,15 +212,12 @@ int main(int argc, char *argv[]) printf("CPU freq (hz): %"PRIu64"\n", odp_sys_cpu_hz()); printf("Cache line size: %i\n", odp_sys_cache_line_size()); printf("Max core count: %i\n", odp_sys_core_count()); - printf("\n"); /* A worker thread per core */ num_workers = odp_sys_core_count(); - if (args.core_count) num_workers = args.core_count; - /* force to max core count */ if (num_workers > MAX_WORKERS) num_workers = MAX_WORKERS; @@ -249,7 +229,6 @@ int main(int argc, char *argv[]) * Start mapping thread from core #1 */ first_core = 1; - if (odp_sys_core_count() == 1) first_core = 0; @@ -271,7 +250,6 @@ int main(int argc, char *argv[]) pool = odp_buffer_pool_create("msg_pool", pool_base, MSG_POOL_SIZE, sizeof(test_message_t), ODP_CACHE_LINE_SIZE, ODP_BUFFER_TYPE_RAW); - if (pool == ODP_BUFFER_POOL_INVALID) { ODP_ERR("Pool create failed.\n"); return -1; @@ -286,7 +264,6 @@ int main(int argc, char *argv[]) param.sched.group = ODP_SCHED_GROUP_DEFAULT; queue = odp_queue_create("timer_queue", ODP_QUEUE_TYPE_SCHED, ¶m); - if (queue == ODP_QUEUE_INVALID) { ODP_ERR("Timer queue create failed.\n"); return -1; @@ -294,8 +271,6 @@ int main(int argc, char *argv[]) test_timer = odp_timer_create("test_timer", pool, 1000000, 1000000, 1000000000000); - - odp_shm_print_all(); printf("CPU freq %"PRIu64" hz\n", odp_sys_cpu_hz()); @@ -329,6 +304,5 @@ int main(int argc, char *argv[]) odp_linux_pthread_join(thread_tbl, num_workers); printf("ODP timer test complete\n\n"); - return 0; }
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- Hello Petri, please review this clean up and merge to your patch. Changes: 1. Doxygen warnings fix. 2. Removed not needed white spaces from timer_test examples which reduced generated documentation and code by 1 page. include/odp_schedule.h | 2 +- platform/linux-generic/source/odp_schedule.c | 1 - test/timer/odp_timer_test.c | 30 ++-------------------------- 3 files changed, 3 insertions(+), 30 deletions(-)