[ccan] [RFC] tal_stack

[Delio Brignoli]

Hi Rusty,

Thanks for reviewing the patch. V2 is attached, see my comments below.

> On 31 Mar 2015, at 02:36, Rusty Russell <rusty at rustcorp.com.au> wrote:
> 
> Delio Brignoli <brignoli.delio at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-tal_stack-new-module-V2.patch
Type: application/octet-stream
Size: 5881 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20150412/5f55d3f5/attachment.obj>
-------------- next part --------------