• Delio Brignoli's avatar
    tal_stack · 73930140
    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: default avatarRusty Russell <rusty@rustcorp.com.au>
    73930140
Makefile-ccan 2.51 KB