Actually, I think the example sketches make excellent comments as they show the intended use of the code.
On Wed, Dec 3, 2014 at 7:47 PM, Mike Holmes <[email protected]> wrote: > > > On 3 December 2014 at 05:02, Ola Liljedahl <[email protected]> > wrote: > >> >> >> diff --git a/example/timer/odp_timer_test.c >> >> >> b/example/timer/odp_timer_test.c >> >> > >> >> > >> >> > We need to rename this, having a "test" in example makes no sense at >> >> > all. >> >> > This patch series should rename this as an example - see below on the >> >> > implication on the large amount of code stored as code in the header >> >> > file >> >> > below, probably that needs to be in the resulting example. >> >> OK. But I wanted to limit the changes introduced by this patch series >> >> and then post additional patches that clean up the testing of the >> >> timer (e.g. odp_timer_ping has to go). >> > >> > >> > Can we capture the work that would still be needed in here 1.0 >> > implementation deltas doc ? >> > I am worried you will skip the country :), but if you can do this soon, >> in >> > the interests of progress I'd be ok with the list in the doc >> Yes, I have added some text on odp_timer_test and odp_timer_ping. >> >> >> > >> >> >> >> >> >> > The "test" portion of this should become a cunit test >> >> OK. But perhaps not in this patch. >> >> >> > >> > Can we capture the work that would still be needed in here 1.0 >> > implementation deltas doc ? >> Ditto. >> >> >> >> > Use C comments /* */ >> >> I can't because this is inside another C comment and those are not >> >> recursive. >> > >> > >> > I had no problem with doxygen or the compiler that I noticed when I >> applied >> > the patch can we take a look ? >> Take a look at what? >> > > I think that before we are done that in this case the example code needs > to be real example code, or the examples broken into snippets with regular > text surrounding them, so I wondered if that were possible to do the latter > in this case. > > Going forward any place that needs a comment in a comment we should treat > it as broken. If an example snippet in a header file requires its own > comments I think it is too complex and must be converted to a real example. > > Maybe that is too strict but having comments in comments just feels wrong. > > >> >> > >> >> >> >> >> >> > Can this example not be linked to the actual example so that the >> example >> >> > in >> >> > the doc is always assured to be as correct as the tests executing it >> in >> >> > CI ? >> >> These are not complete examples so cannot compile. I just want to show >> >> how the API is supposed to be used for a number of common use cases. >> >> Possibly a good example will be able to replace all of these header >> >> file/doxygen fragments. Would that be better? >> > >> > >> > Striving for the best case >> > >> > A good example which runs and that allows doxygen to pull chunks of >> code out >> > to to display would be best. >> > If the code can just compile that might be a better assurance that they >> > don't rot in the header file. >> > >> > But I would stop short of verbatim including it from another file with >> no >> > processing, it may as stay as it is and we can check back after 2.0 to >> see >> > if the examples still work :) >> Yes. >> >> > >> > >> > >> >> >> >> >> >> >> >> > After reviewing further there are a lot of examples in the header >> file I >> >> > am >> >> > sure they will rot unless we link them to the actual example code. >> >> They will rot only if the API is changed (except pure extensions). But >> >> code that actually compiles against the header file is of course >> >> better. >> >> >> >> >> >> > >> >> >> >> >> >> +#define SEC 1000000000ULL //1s expressed in nanoseconds >> >> >> +odp_timer_pool_t tcp_tpid = >> >> >> + odp_timer_pool_create("TCP", >> >> >> + buffer_pool, >> >> >> + 1000000,//resolution 1ms >> >> >> + 0,//min tmo >> >> >> + 7200 * SEC,//max tmo length 2hours >> >> >> + 40000,//num_timers >> >> >> + true,//shared >> >> >> + ODP_CLOCK_CPU >> >> >> + ); >> >> >> +if (tcp_tpid == ODP_TIMER_POOL_INVALID) >> >> >> +{ >> >> >> + //Failed to create timer pool => fatal error >> >> >> +} >> >> >> + >> >> >> + >> >> >> +//Setting up a new connection >> >> >> +//Allocate retransmission timeout (identical for supervision >> timeout) >> >> >> +//The user pointer points back to the connection context >> >> >> +conn->ret_tim = odp_timer_alloc(tcp_tpid, queue, conn); >> >> >> +//Check if all resources were successfully allocated >> >> >> +if (conn->ret_tim == ODP_TIMER_INVALID) >> >> >> +{ >> >> >> + //Failed to allocate all resources for connection => tear >> down >> >> >> + //Tear down connection >> >> >> + ... >> >> >> + return false; >> >> >> +} >> >> >> +//All necessary resources successfully allocated >> >> >> +//Compute initial retransmission length in timer ticks >> >> >> +conn->ret_len = odp_timer_ns_to_tick(tcp_tpid, 3 * SEC);//Per >> RFC1122 >> >> >> +//Allocate a timeout buffer >> >> >> +conn->tmo_buf = odp_buffer_alloc(buffer_pool); >> >> >> +if (conn->tmo_buf == ODP_BUFFER_INVALID) >> >> >> + ODP_ABORT("Failed to allocate timeout buffer\n"); >> >> >> +//Arm the timer with our timeout >> >> >> +conn->expected = odp_timer_set_rel(conn->ret_tim, conn->ret_len, >> >> >> + &conn->tmo_buf); >> >> >> +//Check return value for tooearly or toolate expiration tick >> >> > >> >> > >> >> > too early and too late needs spaces - multiple instances >> >> I was mimicking the symbolic error codes. >> >> From odp_timer.h: >> >> #define ODP_TICK_TOOEARLY 0xFFFFFFFFFFFFFFFDULL >> >> #define ODP_TICK_TOOLATE 0xFFFFFFFFFFFFFFFEULL >> >> >> > >> > Ok, if there is a good reason, it just looked funny to my eye. >> It's a reason, it's subjective whether it's a good reason. But I our >> public identifiers to be constructed by three parts. First the "odp_" >> prefix. Second some kind of module or context identifier, e.g. "TICK" >> here. And third the specific part, either what the function does or >> the error code etc. >> >> ODP_TICK_TOOEARLY vs. ODP_TICK_TOO_EARLY >> ODP_TICK_TOOLATE vs. ODP_TICK_TOO_LATE >> ? >> >> > >> >> >> >> Do you require additional underscores here as well? >> > >> > >> > I think we should use underscores consistently, we really dont across >> the >> > code now and I am not sure what the rule is for when we don't use them, >> if >> > anyone knows I'll add it to the API guide lines doc. >> > >> > I would fix the #defines too. >> OK >> > > I don't have anything else I would hold the patch for, we have capture the > need to fix the actual example and tests. > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp > >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
