On 3 December 2014 at 21:04, Bill Fischofer <[email protected]> wrote:
> Actually, I think the example sketches make excellent comments as they > show the intended use of the code. > Examples are good, no argument at all :) But I think there are cleaner and more robust ways to present them, I would like us to use them. > > 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 >> >> > -- *Mike Holmes* Linaro Sr Technical Manager LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
