| Message ID | 1435938340-7512-4-git-send-email-christophe.milard@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Jul 03, 2015 at 05:45:38PM +0200, Christophe Milard wrote: > As preparation before moving the file (next patch) where the whole > file will be checked again by check-patch called via check-odp. > Some warnings remains about missing blank lines, after variable > declarations, but it would not add to readibility when the > variables are declared in the function's body. > Also a few "line over 80 char" warning remains, but these lines includes > strings which should not be split. > > Signed-off-by: Christophe Milard <christophe.milard@linaro.org> > --- > test/validation/odp_timer.c | 37 ++++++++++++++++++++----------------- > 1 file changed, 20 insertions(+), 17 deletions(-) > > diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c > index 3ab395d..9efb360 100644 > --- a/test/validation/odp_timer.c > +++ b/test/validation/odp_timer.c > @@ -225,25 +225,26 @@ static void handle_tmo(odp_event_t ev, bool stale, uint64_t prev_tick) > > if (tim == ODP_TIMER_INVALID) > CU_FAIL("odp_timeout_timer() invalid timer"); > - if (ttp == NULL) > + if (!ttp) > CU_FAIL("odp_timeout_user_ptr() null user ptr"); > > - if (ttp != NULL && ttp->ev2 != ev) > + if (ttp && ttp->ev2 != ev) > CU_FAIL("odp_timeout_user_ptr() wrong user ptr"); > - if (ttp != NULL && ttp->tim != tim) > + if (ttp && ttp->tim != tim) > CU_FAIL("odp_timeout_timer() wrong timer"); > if (stale) { > if (odp_timeout_fresh(tmo)) > CU_FAIL("Wrong status (fresh) for stale timeout"); > /* Stale timeout => local timer must have invalid tick */ > - if (ttp != NULL && ttp->tick != TICK_INVALID) > + if (ttp && ttp->tick != TICK_INVALID) > CU_FAIL("Stale timeout for active timer"); > } else { > if (!odp_timeout_fresh(tmo)) > CU_FAIL("Wrong status (stale) for fresh timeout"); > /* Fresh timeout => local timer must have matching tick */ > - if (ttp != NULL && ttp->tick != tick) { > - LOG_DBG("Wrong tick: expected %"PRIu64" actual %"PRIu64"\n", > + if (ttp && ttp->tick != tick) { > + LOG_DBG("Wrong tick: expected %" PRIu64 > + " actual %" PRIu64 "\n", I think this would be better off left unwrapped. > ttp->tick, tick); > CU_FAIL("odp_timeout_tick() wrong tick"); > } > @@ -251,14 +252,15 @@ static void handle_tmo(odp_event_t ev, bool stale, uint64_t prev_tick) > if (tick > odp_timer_current_tick(tp)) > CU_FAIL("Timeout delivered early"); > if (tick < prev_tick) { > - LOG_DBG("Too late tick: %"PRIu64" prev_tick %"PRIu64"\n", > + LOG_DBG("Too late tick: %" PRIu64 > + " prev_tick %" PRIu64"\n", And here. I thought checkpatch had been modified to ignore long lines for debug macros, but perhaps this isn't working as it does WARN on another LOG_DBG further down.. > tick, prev_tick); > /* We don't report late timeouts using CU_FAIL */ > odp_atomic_inc_u32(&ndelivtoolate); > } > } > > - if (ttp != NULL) { > + if (ttp) { > /* Internal error */ > CU_ASSERT_FATAL(ttp->ev == ODP_EVENT_INVALID); > ttp->ev = ev; > @@ -281,7 +283,7 @@ static void *worker_entrypoint(void *arg TEST_UNUSED) > CU_FAIL_FATAL("Queue create failed"); > > struct test_timer *tt = malloc(sizeof(struct test_timer) * NTIMERS); > - if (tt == NULL) > + if (!tt) > CU_FAIL_FATAL("malloc failed"); > > /* Prepare all timers */ > @@ -322,6 +324,7 @@ static void *worker_entrypoint(void *arg TEST_UNUSED) > uint32_t ntoolate = 0; > uint32_t ms; > uint64_t prev_tick = odp_timer_current_tick(tp); > + > for (ms = 0; ms < 7 * RANGE_MS / 10; ms++) { > odp_event_t ev; > while ((ev = odp_queue_deq(queue)) != ODP_EVENT_INVALID) { > @@ -390,13 +393,13 @@ static void *worker_entrypoint(void *arg TEST_UNUSED) > CU_FAIL("odp_timer_free"); > } > > - LOG_DBG("Thread %u: %"PRIu32" timers set\n", thr, nset); > - LOG_DBG("Thread %u: %"PRIu32" timers reset\n", thr, nreset); > - LOG_DBG("Thread %u: %"PRIu32" timers cancelled\n", thr, ncancel); > - LOG_DBG("Thread %u: %"PRIu32" timers reset/cancelled too late\n", > + LOG_DBG("Thread %u: %" PRIu32 " timers set\n", thr, nset); > + LOG_DBG("Thread %u: %" PRIu32 " timers reset\n", thr, nreset); > + LOG_DBG("Thread %u: %" PRIu32 " timers cancelled\n", thr, ncancel); > + LOG_DBG("Thread %u: %" PRIu32 " timers reset/cancelled too late\n", > thr, ntoolate); > - LOG_DBG("Thread %u: %"PRIu32" timeouts received\n", thr, nrcv); > - LOG_DBG("Thread %u: %"PRIu32" stale timeout(s) after odp_timer_free()\n", > + LOG_DBG("Thread %u: %" PRIu32 " timeouts received\n", thr, nrcv); > + LOG_DBG("Thread %u: %" PRIu32 " stale timeout(s) after odp_timer_free()\n", > thr, nstale); > > /* Delay some more to ensure timeouts for expired timers can be > @@ -482,7 +485,7 @@ static void timer_test_odp_timer_all(void) > CU_ASSERT(strcmp(tpinfo.name, NAME) == 0); > > LOG_DBG("#timers..: %u\n", NTIMERS); > - LOG_DBG("Tmo range: %u ms (%"PRIu64" ticks)\n", RANGE_MS, > + LOG_DBG("Tmo range: %u ms (%" PRIu64 " ticks)\n", RANGE_MS, > odp_timer_ns_to_tick(tp, 1000000ULL * RANGE_MS)); > > uint64_t tick; > @@ -507,7 +510,7 @@ static void timer_test_odp_timer_all(void) > > /* Wait for worker threads to exit */ > odp_cunit_thread_exit(&thrdarg); > - LOG_DBG("Number of timeouts delivered/received too late: %"PRIu32"\n", > + LOG_DBG("Number of timeouts delivered/received too late: %" PRIu32 "\n", > odp_atomic_load_u32(&ndelivtoolate)); > > /* Check some statistics after the test */ > -- > 1.9.1 >
It did warn me. This is the reason why I broke some of these lines when it looked reasonable. What do you wish me to do? Christophe. On 8 July 2015 at 14:31, Stuart Haslam <stuart.haslam@linaro.org> wrote: > On Fri, Jul 03, 2015 at 05:45:38PM +0200, Christophe Milard wrote: > > As preparation before moving the file (next patch) where the whole > > file will be checked again by check-patch called via check-odp. > > Some warnings remains about missing blank lines, after variable > > declarations, but it would not add to readibility when the > > variables are declared in the function's body. > > Also a few "line over 80 char" warning remains, but these lines includes > > strings which should not be split. > > > > Signed-off-by: Christophe Milard <christophe.milard@linaro.org> > > --- > > test/validation/odp_timer.c | 37 ++++++++++++++++++++----------------- > > 1 file changed, 20 insertions(+), 17 deletions(-) > > > > diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c > > index 3ab395d..9efb360 100644 > > --- a/test/validation/odp_timer.c > > +++ b/test/validation/odp_timer.c > > @@ -225,25 +225,26 @@ static void handle_tmo(odp_event_t ev, bool stale, > uint64_t prev_tick) > > > > if (tim == ODP_TIMER_INVALID) > > CU_FAIL("odp_timeout_timer() invalid timer"); > > - if (ttp == NULL) > > + if (!ttp) > > CU_FAIL("odp_timeout_user_ptr() null user ptr"); > > > > - if (ttp != NULL && ttp->ev2 != ev) > > + if (ttp && ttp->ev2 != ev) > > CU_FAIL("odp_timeout_user_ptr() wrong user ptr"); > > - if (ttp != NULL && ttp->tim != tim) > > + if (ttp && ttp->tim != tim) > > CU_FAIL("odp_timeout_timer() wrong timer"); > > if (stale) { > > if (odp_timeout_fresh(tmo)) > > CU_FAIL("Wrong status (fresh) for stale timeout"); > > /* Stale timeout => local timer must have invalid tick */ > > - if (ttp != NULL && ttp->tick != TICK_INVALID) > > + if (ttp && ttp->tick != TICK_INVALID) > > CU_FAIL("Stale timeout for active timer"); > > } else { > > if (!odp_timeout_fresh(tmo)) > > CU_FAIL("Wrong status (stale) for fresh timeout"); > > /* Fresh timeout => local timer must have matching tick */ > > - if (ttp != NULL && ttp->tick != tick) { > > - LOG_DBG("Wrong tick: expected %"PRIu64" actual > %"PRIu64"\n", > > + if (ttp && ttp->tick != tick) { > > + LOG_DBG("Wrong tick: expected %" PRIu64 > > + " actual %" PRIu64 "\n", > > I think this would be better off left unwrapped. > > > ttp->tick, tick); > > CU_FAIL("odp_timeout_tick() wrong tick"); > > } > > @@ -251,14 +252,15 @@ static void handle_tmo(odp_event_t ev, bool stale, > uint64_t prev_tick) > > if (tick > odp_timer_current_tick(tp)) > > CU_FAIL("Timeout delivered early"); > > if (tick < prev_tick) { > > - LOG_DBG("Too late tick: %"PRIu64" prev_tick > %"PRIu64"\n", > > + LOG_DBG("Too late tick: %" PRIu64 > > + " prev_tick %" PRIu64"\n", > > And here. > > I thought checkpatch had been modified to ignore long lines for debug > macros, but perhaps this isn't working as it does WARN on another > LOG_DBG further down.. > > > tick, prev_tick); > > /* We don't report late timeouts using CU_FAIL */ > > odp_atomic_inc_u32(&ndelivtoolate); > > } > > } > > > > - if (ttp != NULL) { > > + if (ttp) { > > /* Internal error */ > > CU_ASSERT_FATAL(ttp->ev == ODP_EVENT_INVALID); > > ttp->ev = ev; > > @@ -281,7 +283,7 @@ static void *worker_entrypoint(void *arg TEST_UNUSED) > > CU_FAIL_FATAL("Queue create failed"); > > > > struct test_timer *tt = malloc(sizeof(struct test_timer) * > NTIMERS); > > - if (tt == NULL) > > + if (!tt) > > CU_FAIL_FATAL("malloc failed"); > > > > /* Prepare all timers */ > > @@ -322,6 +324,7 @@ static void *worker_entrypoint(void *arg TEST_UNUSED) > > uint32_t ntoolate = 0; > > uint32_t ms; > > uint64_t prev_tick = odp_timer_current_tick(tp); > > + > > for (ms = 0; ms < 7 * RANGE_MS / 10; ms++) { > > odp_event_t ev; > > while ((ev = odp_queue_deq(queue)) != ODP_EVENT_INVALID) { > > @@ -390,13 +393,13 @@ static void *worker_entrypoint(void *arg > TEST_UNUSED) > > CU_FAIL("odp_timer_free"); > > } > > > > - LOG_DBG("Thread %u: %"PRIu32" timers set\n", thr, nset); > > - LOG_DBG("Thread %u: %"PRIu32" timers reset\n", thr, nreset); > > - LOG_DBG("Thread %u: %"PRIu32" timers cancelled\n", thr, ncancel); > > - LOG_DBG("Thread %u: %"PRIu32" timers reset/cancelled too late\n", > > + LOG_DBG("Thread %u: %" PRIu32 " timers set\n", thr, nset); > > + LOG_DBG("Thread %u: %" PRIu32 " timers reset\n", thr, nreset); > > + LOG_DBG("Thread %u: %" PRIu32 " timers cancelled\n", thr, ncancel); > > + LOG_DBG("Thread %u: %" PRIu32 " timers reset/cancelled too late\n", > > thr, ntoolate); > > - LOG_DBG("Thread %u: %"PRIu32" timeouts received\n", thr, nrcv); > > - LOG_DBG("Thread %u: %"PRIu32" stale timeout(s) after > odp_timer_free()\n", > > + LOG_DBG("Thread %u: %" PRIu32 " timeouts received\n", thr, nrcv); > > + LOG_DBG("Thread %u: %" PRIu32 " stale timeout(s) after > odp_timer_free()\n", > > thr, nstale); > > > > /* Delay some more to ensure timeouts for expired timers can be > > @@ -482,7 +485,7 @@ static void timer_test_odp_timer_all(void) > > CU_ASSERT(strcmp(tpinfo.name, NAME) == 0); > > > > LOG_DBG("#timers..: %u\n", NTIMERS); > > - LOG_DBG("Tmo range: %u ms (%"PRIu64" ticks)\n", RANGE_MS, > > + LOG_DBG("Tmo range: %u ms (%" PRIu64 " ticks)\n", RANGE_MS, > > odp_timer_ns_to_tick(tp, 1000000ULL * RANGE_MS)); > > > > uint64_t tick; > > @@ -507,7 +510,7 @@ static void timer_test_odp_timer_all(void) > > > > /* Wait for worker threads to exit */ > > odp_cunit_thread_exit(&thrdarg); > > - LOG_DBG("Number of timeouts delivered/received too late: > %"PRIu32"\n", > > + LOG_DBG("Number of timeouts delivered/received too late: %" PRIu32 > "\n", > > odp_atomic_load_u32(&ndelivtoolate)); > > > > /* Check some statistics after the test */ > > -- > > 1.9.1 > > > > -- > Stuart. >
On Wed, Jul 08, 2015 at 02:37:01PM +0200, Christophe Milard wrote: > It did warn me. This is the reason why I broke some of these lines when it > looked reasonable. What do you wish me to do? Not wrap the line in the two places I highlighted. As a secondary thing we should also look into why checkpatch is warning on these as it shouldn't be AFAICT (I did have a quick look but the regex there made my eyes bleed). -- Stuart.
diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c index 3ab395d..9efb360 100644 --- a/test/validation/odp_timer.c +++ b/test/validation/odp_timer.c @@ -225,25 +225,26 @@ static void handle_tmo(odp_event_t ev, bool stale, uint64_t prev_tick) if (tim == ODP_TIMER_INVALID) CU_FAIL("odp_timeout_timer() invalid timer"); - if (ttp == NULL) + if (!ttp) CU_FAIL("odp_timeout_user_ptr() null user ptr"); - if (ttp != NULL && ttp->ev2 != ev) + if (ttp && ttp->ev2 != ev) CU_FAIL("odp_timeout_user_ptr() wrong user ptr"); - if (ttp != NULL && ttp->tim != tim) + if (ttp && ttp->tim != tim) CU_FAIL("odp_timeout_timer() wrong timer"); if (stale) { if (odp_timeout_fresh(tmo)) CU_FAIL("Wrong status (fresh) for stale timeout"); /* Stale timeout => local timer must have invalid tick */ - if (ttp != NULL && ttp->tick != TICK_INVALID) + if (ttp && ttp->tick != TICK_INVALID) CU_FAIL("Stale timeout for active timer"); } else { if (!odp_timeout_fresh(tmo)) CU_FAIL("Wrong status (stale) for fresh timeout"); /* Fresh timeout => local timer must have matching tick */ - if (ttp != NULL && ttp->tick != tick) { - LOG_DBG("Wrong tick: expected %"PRIu64" actual %"PRIu64"\n", + if (ttp && ttp->tick != tick) { + LOG_DBG("Wrong tick: expected %" PRIu64 + " actual %" PRIu64 "\n", ttp->tick, tick); CU_FAIL("odp_timeout_tick() wrong tick"); } @@ -251,14 +252,15 @@ static void handle_tmo(odp_event_t ev, bool stale, uint64_t prev_tick) if (tick > odp_timer_current_tick(tp)) CU_FAIL("Timeout delivered early"); if (tick < prev_tick) { - LOG_DBG("Too late tick: %"PRIu64" prev_tick %"PRIu64"\n", + LOG_DBG("Too late tick: %" PRIu64 + " prev_tick %" PRIu64"\n", tick, prev_tick); /* We don't report late timeouts using CU_FAIL */ odp_atomic_inc_u32(&ndelivtoolate); } } - if (ttp != NULL) { + if (ttp) { /* Internal error */ CU_ASSERT_FATAL(ttp->ev == ODP_EVENT_INVALID); ttp->ev = ev; @@ -281,7 +283,7 @@ static void *worker_entrypoint(void *arg TEST_UNUSED) CU_FAIL_FATAL("Queue create failed"); struct test_timer *tt = malloc(sizeof(struct test_timer) * NTIMERS); - if (tt == NULL) + if (!tt) CU_FAIL_FATAL("malloc failed"); /* Prepare all timers */ @@ -322,6 +324,7 @@ static void *worker_entrypoint(void *arg TEST_UNUSED) uint32_t ntoolate = 0; uint32_t ms; uint64_t prev_tick = odp_timer_current_tick(tp); + for (ms = 0; ms < 7 * RANGE_MS / 10; ms++) { odp_event_t ev; while ((ev = odp_queue_deq(queue)) != ODP_EVENT_INVALID) { @@ -390,13 +393,13 @@ static void *worker_entrypoint(void *arg TEST_UNUSED) CU_FAIL("odp_timer_free"); } - LOG_DBG("Thread %u: %"PRIu32" timers set\n", thr, nset); - LOG_DBG("Thread %u: %"PRIu32" timers reset\n", thr, nreset); - LOG_DBG("Thread %u: %"PRIu32" timers cancelled\n", thr, ncancel); - LOG_DBG("Thread %u: %"PRIu32" timers reset/cancelled too late\n", + LOG_DBG("Thread %u: %" PRIu32 " timers set\n", thr, nset); + LOG_DBG("Thread %u: %" PRIu32 " timers reset\n", thr, nreset); + LOG_DBG("Thread %u: %" PRIu32 " timers cancelled\n", thr, ncancel); + LOG_DBG("Thread %u: %" PRIu32 " timers reset/cancelled too late\n", thr, ntoolate); - LOG_DBG("Thread %u: %"PRIu32" timeouts received\n", thr, nrcv); - LOG_DBG("Thread %u: %"PRIu32" stale timeout(s) after odp_timer_free()\n", + LOG_DBG("Thread %u: %" PRIu32 " timeouts received\n", thr, nrcv); + LOG_DBG("Thread %u: %" PRIu32 " stale timeout(s) after odp_timer_free()\n", thr, nstale); /* Delay some more to ensure timeouts for expired timers can be @@ -482,7 +485,7 @@ static void timer_test_odp_timer_all(void) CU_ASSERT(strcmp(tpinfo.name, NAME) == 0); LOG_DBG("#timers..: %u\n", NTIMERS); - LOG_DBG("Tmo range: %u ms (%"PRIu64" ticks)\n", RANGE_MS, + LOG_DBG("Tmo range: %u ms (%" PRIu64 " ticks)\n", RANGE_MS, odp_timer_ns_to_tick(tp, 1000000ULL * RANGE_MS)); uint64_t tick; @@ -507,7 +510,7 @@ static void timer_test_odp_timer_all(void) /* Wait for worker threads to exit */ odp_cunit_thread_exit(&thrdarg); - LOG_DBG("Number of timeouts delivered/received too late: %"PRIu32"\n", + LOG_DBG("Number of timeouts delivered/received too late: %" PRIu32 "\n", odp_atomic_load_u32(&ndelivtoolate)); /* Check some statistics after the test */
As preparation before moving the file (next patch) where the whole file will be checked again by check-patch called via check-odp. Some warnings remains about missing blank lines, after variable declarations, but it would not add to readibility when the variables are declared in the function's body. Also a few "line over 80 char" warning remains, but these lines includes strings which should not be split. Signed-off-by: Christophe Milard <christophe.milard@linaro.org> --- test/validation/odp_timer.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-)