[PATCH] util: Add L_AUTO_FREE_OBJ and l_steal_ptr macros

[PATCH] util: Add L_AUTO_FREE_OBJ and l_steal_ptr macros

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]

L_AUTO_FREE_OBJ can be used for ell object types that define a
destructor named l_<type>_free such as l_settings_free().  The "FREE" in
the name hopefully is enough to communicate this.

Some of our classes use l_<type>_destroy and similar names and some of
those functions, like l_queue_destroy, take additional parameters so for
those classes if a similar "AUTO" macros is wanted, it may be better
defined directly in that class's header.

l_steal_ptr is similar to g_steal_pointer except for being a macro.
Both macros can be used like in this example:

struct l_settings *load_settings(const char *path)
{
	L_AUTO_FREE_OBJ(l_settings, s) = l_settings_new();

	if (!l_settings_load_from_file(s, path))
		return NULL;

	return l_steal_ptr(s);
}
---
This is an RFC.  L_AUTO_FREE_OBJ is based on James's idea.

 ell/util.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ell/util.h b/ell/util.h
index b112878..94a4178 100644
--- a/ell/util.h
+++ b/ell/util.h
@@ -235,6 +235,11 @@ static inline void l_put_be64(uint64_t val, void *ptr)
 #define L_AUTO_FREE_VAR(vartype,varname) \
 	vartype varname __attribute__((cleanup(auto_free)))
 
+#define L_AUTO_FREE_OBJ(l_type,varname) \
+	void varname ## _auto_free(void *_ptr)		\
+		{ l_type ## _free(*(void **) _ptr); }	\
+	L_AUTO_CLEANUP_VAR(struct l_type *, varname, varname ## _auto_free)
+
 #define L_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
 void *l_malloc(size_t size) __attribute__ ((warn_unused_result, malloc));
@@ -276,6 +281,9 @@ static inline size_t minsize(size_t a, size_t b)
 		__p;				\
 	}))
 
+#define l_steal_ptr(ptr)	\
+	(__extension__ ({ typeof(ptr) *_tmp = (ptr); (ptr) = NULL; _tmp; }))
+
 char *l_strdup(const char *str);
 char *l_strndup(const char *str, size_t max);
 char *l_strdup_printf(const char *format, ...)
-- 
2.27.0

Re: [PATCH] util: Add L_AUTO_FREE_OBJ and l_steal_ptr macros

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3030 bytes --]

Hi Andrew,

On 3/4/21 3:53 PM, Andrew Zaborowski wrote:
> L_AUTO_FREE_OBJ can be used for ell object types that define a
> destructor named l_<type>_free such as l_settings_free().  The "FREE" in
> the name hopefully is enough to communicate this.
> 
> Some of our classes use l_<type>_destroy and similar names and some of
> those functions, like l_queue_destroy, take additional parameters so for
> those classes if a similar "AUTO" macros is wanted, it may be better
> defined directly in that class's header.
> 

I'd rather have the caller provide the destructor method itself rather than the 
class.  That becomes a bit more generic as well.

> l_steal_ptr is similar to g_steal_pointer except for being a macro.
> Both macros can be used like in this example:
> 

No objections from me

> struct l_settings *load_settings(const char *path)
> {
> 	L_AUTO_FREE_OBJ(l_settings, s) = l_settings_new();
> 
> 	if (!l_settings_load_from_file(s, path))
> 		return NULL;
> 
> 	return l_steal_ptr(s);
> }
> ---
> This is an RFC.  L_AUTO_FREE_OBJ is based on James's idea.
> 
>   ell/util.h | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/ell/util.h b/ell/util.h
> index b112878..94a4178 100644
> --- a/ell/util.h
> +++ b/ell/util.h
> @@ -235,6 +235,11 @@ static inline void l_put_be64(uint64_t val, void *ptr)
>   #define L_AUTO_FREE_VAR(vartype,varname) \
>   	vartype varname __attribute__((cleanup(auto_free)))
>   
> +#define L_AUTO_FREE_OBJ(l_type,varname) \
> +	void varname ## _auto_free(void *_ptr)		\
> +		{ l_type ## _free(*(void **) _ptr); }	\
> +	L_AUTO_CLEANUP_VAR(struct l_type *, varname, varname ## _auto_free)
> +

Nested functions... Very clever!

I ran a few experiments by changing out the L_AUTO_FREE_VAR macro to use the 
above pattern.  Something like:

#define L_AUTO_FREE_VAR(vartype,varname) \
void varname ## _auto_free(void *_ptr) \
{ l_free(*(void **) _ptr); } \
vartype varname __attribute__((cleanup(varname ## _auto_free)))

The resulting iwd code was the same size as the original, which seems to 
indicate that the nested function is always inlined.

I also made some (somewhat primitive) experiments with writing several versions 
of a simple function, both using this cleanup pattern and code without any GCC 
extensions.  Both seemed to produce the same code after disassembly.

Curious to see whether you or anyone else has different findings?

Given the above and the fact that L_AUTO_CLEANUP_VAR was never really used (it 
is used in ell once), my vote would be to simply hijack that macro.  Then we can 
write things like:

L_AUTO_CLEANUP_VAR(struct l_settings *, settings, l_settings_free);

Or perhaps something like:

_cleanup(l_settings_free) struct l_settings *settings;

if we can use a UNIQUE_NAME type macro magic inside.

>   #define L_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>   
>   void *l_malloc(size_t size) __attribute__ ((warn_unused_result, malloc));

Regards,
-Denis

Re: [PATCH] util: Add L_AUTO_FREE_OBJ and l_steal_ptr macros

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3734 bytes --]

Hi Denis,

On Fri, 5 Mar 2021 at 22:02, Denis Kenzior <denkenz@gmail.com> wrote:
> On 3/4/21 3:53 PM, Andrew Zaborowski wrote:
> > L_AUTO_FREE_OBJ can be used for ell object types that define a
> > destructor named l_<type>_free such as l_settings_free().  The "FREE" in
> > the name hopefully is enough to communicate this.
> >
> > Some of our classes use l_<type>_destroy and similar names and some of
> > those functions, like l_queue_destroy, take additional parameters so for
> > those classes if a similar "AUTO" macros is wanted, it may be better
> > defined directly in that class's header.
> >
>
> I'd rather have the caller provide the destructor method itself rather than the
> class.  That becomes a bit more generic as well.
>
> > l_steal_ptr is similar to g_steal_pointer except for being a macro.
> > Both macros can be used like in this example:
> >
>
> No objections from me
>
> > struct l_settings *load_settings(const char *path)
> > {
> >       L_AUTO_FREE_OBJ(l_settings, s) = l_settings_new();
> >
> >       if (!l_settings_load_from_file(s, path))
> >               return NULL;
> >
> >       return l_steal_ptr(s);
> > }
> > ---
> > This is an RFC.  L_AUTO_FREE_OBJ is based on James's idea.
> >
> >   ell/util.h | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/ell/util.h b/ell/util.h
> > index b112878..94a4178 100644
> > --- a/ell/util.h
> > +++ b/ell/util.h
> > @@ -235,6 +235,11 @@ static inline void l_put_be64(uint64_t val, void *ptr)
> >   #define L_AUTO_FREE_VAR(vartype,varname) \
> >       vartype varname __attribute__((cleanup(auto_free)))
> >
> > +#define L_AUTO_FREE_OBJ(l_type,varname) \
> > +     void varname ## _auto_free(void *_ptr)          \
> > +             { l_type ## _free(*(void **) _ptr); }   \
> > +     L_AUTO_CLEANUP_VAR(struct l_type *, varname, varname ## _auto_free)
> > +
>
> Nested functions... Very clever!
>
> I ran a few experiments by changing out the L_AUTO_FREE_VAR macro to use the
> above pattern.  Something like:
>
> #define L_AUTO_FREE_VAR(vartype,varname) \
> void varname ## _auto_free(void *_ptr) \
> { l_free(*(void **) _ptr); } \
> vartype varname __attribute__((cleanup(varname ## _auto_free)))
>
> The resulting iwd code was the same size as the original, which seems to
> indicate that the nested function is always inlined.
>
> I also made some (somewhat primitive) experiments with writing several versions
> of a simple function, both using this cleanup pattern and code without any GCC
> extensions.  Both seemed to produce the same code after disassembly.
>
> Curious to see whether you or anyone else has different findings?

I haven't looked at the code size honestly.
>
> Given the above and the fact that L_AUTO_CLEANUP_VAR was never really used (it
> is used in ell once), my vote would be to simply hijack that macro.  Then we can
> write things like:
>
> L_AUTO_CLEANUP_VAR(struct l_settings *, settings, l_settings_free);

Re-using L_AUTO_CLEANUP_VAR makes sense but I think the name and
parameters are too long for it to be attractive to use.  It would
mainly make sense if the individual classes' headers use it to define
their own macros.  On the other hand in that scenario the headers can
define global cleanup functions without using the nested function
magic.

I see one of the following forms as most usable:

l_settings_auto_ptr settings;
l_settings_auto_ptr *settings;
l_auto_ptr(l_settings) settings;
l_auto_ptr(l_settings) *settings;

The l_auto_ptr() macro would rely on settings.h defining a cleanup
function whose name can be derived by preppending and/or appending
something to "l_settings".

Best regards

Re: [PATCH] util: Add L_AUTO_FREE_OBJ and l_steal_ptr macros

[Phil]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

It might be good to avoid any possible confusion with the C++03 auto_ptr, which is deprecated in C++11 favour of unique_ptr.

> I see one of the following forms as most usable:
> 
> l_settings_auto_ptr settings;
> l_settings_auto_ptr *settings;
> l_auto_ptr(l_settings) settings;
> l_auto_ptr(l_settings) *settings;
> 
> The l_auto_ptr() macro would rely on settings.h defining a cleanup
> function whose name can be derived by preppending and/or appending
> something to "l_settings".

Re: [PATCH] util: Add L_AUTO_FREE_OBJ and l_steal_ptr macros

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

Hi Andrew,

>> Given the above and the fact that L_AUTO_CLEANUP_VAR was never really used (it
>> is used in ell once), my vote would be to simply hijack that macro.  Then we can
>> write things like:
>>
>> L_AUTO_CLEANUP_VAR(struct l_settings *, settings, l_settings_free);
> 
> Re-using L_AUTO_CLEANUP_VAR makes sense but I think the name and
> parameters are too long for it to be attractive to use.  It would

Yes.  Ideally we could do _cleanup() instead.

> mainly make sense if the individual classes' headers use it to define
> their own macros.  On the other hand in that scenario the headers can
> define global cleanup functions without using the nested function
> magic.

I find this less desirable actually.  The nice thing about the nested function 
approach is that you can easily use it for custom classes without needing to use 
any extra macros.

> 
> I see one of the following forms as most usable:
> 
> l_settings_auto_ptr settings;
> l_settings_auto_ptr *settings;
> l_auto_ptr(l_settings) settings;
> l_auto_ptr(l_settings) *settings;

auto_ptr is some C++-ism though, so that might not be the best candidate.

> 
> The l_auto_ptr() macro would rely on settings.h defining a cleanup
> function whose name can be derived by preppending and/or appending
> something to "l_settings".

And this is another down side.

How about something like:
#define __CLEANUP(var, func)                           \
         void cleanup_ ## var(void *ptr)                \
         { func(*(void **) ptr); }                      \
         __attribute((cleanup(cleanup_ ## var)))

#define _CLEANUP(var, func)                            \
         __CLEANUP(var, func)

#define _cleanup(func)                                 \
         _CLEANUP(__COUNTER__, func)

void foo()
{
	_cleanup(destructor) struct something *s =
					constructor();
}

Regards,
-Denis

Re: [PATCH] util: Add L_AUTO_FREE_OBJ and l_steal_ptr macros

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]

On Mon, 8 Mar 2021 at 21:02, Denis Kenzior <denkenz@gmail.com> wrote:
>
> > mainly make sense if the individual classes' headers use it to define
> > their own macros.  On the other hand in that scenario the headers can
> > define global cleanup functions without using the nested function
> > magic.
>
> I find this less desirable actually.  The nice thing about the nested function
> approach is that you can easily use it for custom classes without needing to use
> any extra macros.

Right, but custom classes are one use case and ell classes are
another.  We can add both a generic macro and an ell-specific or
class-specific (e.g. l_settings-only) extra compact syntax.

>
> >
> > I see one of the following forms as most usable:
> >
> > l_settings_auto_ptr settings;
> > l_settings_auto_ptr *settings;
> > l_auto_ptr(l_settings) settings;
> > l_auto_ptr(l_settings) *settings;
>
> auto_ptr is some C++-ism though, so that might not be the best candidate.

Right, I didn't remember that until Phil mentioned it, I take back
these name ideas.

>
> >
> > The l_auto_ptr() macro would rely on settings.h defining a cleanup
> > function whose name can be derived by preppending and/or appending
> > something to "l_settings".
>
> And this is another down side.
>
> How about something like:
> #define __CLEANUP(var, func)                           \
>          void cleanup_ ## var(void *ptr)                \
>          { func(*(void **) ptr); }                      \
>          __attribute((cleanup(cleanup_ ## var)))
>
> #define _CLEANUP(var, func)                            \
>          __CLEANUP(var, func)
>
> #define _cleanup(func)                                 \
>          _CLEANUP(__COUNTER__, func)
>
> void foo()
> {
>         _cleanup(destructor) struct something *s =
>                                         constructor();
> }

Make sense, one comment though:

I guess with _cleanup you want to make it shorter by skipping the "l",
but for the internal macros there's no reason not to make them more
descriptive (e.g. something that hints on how _CLEANUP and __CLEANUP
differ from _cleanup and from each other) and perhaps also use the L_
prefix to tidiness.  All three are pretty short names and not unlikely
for someone to be using already.

Best regards

Re: [PATCH] util: Add L_AUTO_FREE_OBJ and l_steal_ptr macros

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

Hi Andrew,

>> I find this less desirable actually.  The nice thing about the nested function
>> approach is that you can easily use it for custom classes without needing to use
>> any extra macros.
> 
> Right, but custom classes are one use case and ell classes are
> another.  We can add both a generic macro and an ell-specific or
> class-specific (e.g. l_settings-only) extra compact syntax.
> 

We can.  But if we can have a single macro that works for everything, I'd 
consider that to be a better approach.

>> How about something like:
>> #define __CLEANUP(var, func)                           \
>>           void cleanup_ ## var(void *ptr)                \
>>           { func(*(void **) ptr); }                      \
>>           __attribute((cleanup(cleanup_ ## var)))
>>
>> #define _CLEANUP(var, func)                            \
>>           __CLEANUP(var, func)
>>
>> #define _cleanup(func)                                 \
>>           _CLEANUP(__COUNTER__, func)
>>
>> void foo()
>> {
>>          _cleanup(destructor) struct something *s =
>>                                          constructor();
>> }
> 
> Make sense, one comment though:
> 
> I guess with _cleanup you want to make it shorter by skipping the "l",

Not really.  We can make it l_cleanup or l_auto or whichever...  I'm not 
particular to anything.  This was mostly a proof of concept

> but for the internal macros there's no reason not to make them more
> descriptive (e.g. something that hints on how _CLEANUP and __CLEANUP
> differ from _cleanup and from each other) and perhaps also use the L_
> prefix to tidiness.  All three are pretty short names and not unlikely
> for someone to be using already.

Sure.

Regards,
-Denis