-
Delio Brignoli authored
Hi Rusty, Thanks for reviewing the patch. V2 is attached, see my comments below. > On 31 Mar 2015, at 02:36, Rusty Russell <rusty@rustcorp.com.au> wrote: > > Delio Brignoli <brignoli.delio@gmail.com> writes: >> Hi All, >> >> tal_stack implements a (trivial) stack of tal contexts. Would this be a worthy addition to CCAN? (not necessarily in its current form). […] > This is cute; I’ve seen similar used in Samba. It's Indeed, it was inspired by talloc_stack.h ;-) […] > You are missing a _info file: I would create that, and put your example > in an Example: section there. I moved the module and tests under can/tal/stack and added a LICENSE and _info. > Other random advice: > 1) You should also document the tal_newframe function (particularly note > that you're expected to tal_free the result, and that it will free > any future unfreed frames). And note that it’s not threadsafe. Done. > 2) You probably want tal_newframe to be a macro, and hand file and line > thought to the tal_alloc_ call. That makes debugging nicer when > you iterate the tree. Done. The macro is calling a tal_newframe_() function because I’d rather not make the module’s stack variable ‘public’. > 3) Consider whether you want to declare a dummy type 'struct tal_stack'. > Probably pretty unnecessary since it’s quite clear. Skipped this one. We can declare it later if we change our minds. Thanks — Delio From c2ceb9258d97b0dcb72e7b6986cfd2bd394b254e Mon Sep 17 00:00:00 2001 From: Delio Brignoli <dbrignoli@audioscience.com> Date: Sun, 15 Mar 2015 13:26:40 +0100 Subject: [PATCH] tal_stack: new module - V2 Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
73930140