[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #24337 [Core Tor/Tor]: Every _free() function should be a macro that sets the corresponding pointer to NULL.
#24337: Every _free() function should be a macro that sets the corresponding
pointer to NULL.
----------------------------------------------+----------------------------
Reporter: nickm | Owner: nickm
Type: enhancement | Status:
| merge_ready
Priority: Medium | Milestone: Tor:
| 0.3.3.x-final
Component: Core Tor/Tor | Version:
Severity: Normal | Resolution:
Keywords: review-group-26, review-group-27 | Actual Points:
Parent ID: | Points:
Reviewer: dgoulet | Sponsor:
| Sponsor8-can
----------------------------------------------+----------------------------
Changes (by dgoulet):
* status: needs_review => merge_ready
Comment:
Ok I guess we are good to go. Compiles and `make check` passes. However,
it is based on a head commit for which `test-network-all` is broken so I
can't really test that part.
Last thing, let us make a note at the next weekly meeting to "announce" to
everyone that this is a thing now because creating free() functions just
became a bit more complex than just `tor_free()` :).
Extra idea for helping us but doesn't block this branch getting merged:
What about we add an helper macro to declare/implement free functions
which would use a standard naming convention (and we could have one that
takes a `free_fn` for special cases:
{{{
#define DECL_FREE(name, typename) \
void name##_(typename *__obj); \
static inline void \
name(typename *__obj) { \
FREE_AND_NULL(typename, name##_, (__obj)); \
}
}}}
As an example with hs_service_free():
{{{
+ DECL_FREE(hs_service_free, hs_service_t)
- void hs_service_free_(hs_service_t *service);
- #define hs_service_free(s) FREE_AND_NULL(hs_service_t, hs_service_free_,
(s))
}}}
It makes the `hs_service_free()` inline, then declares the
`hs_service_free_()` which needs to be implemented in a .c file for which
we could use `IMPL_FREE(hs_service_free, hs_service_t)` kind of macro for
the signature.
The thing I like about this is that it completely hides the fact that a
second symbol with a trailing underscore exists and thus forces us to
always use and consider `hs_service_free` instead of `hs_service_free_`.
We could go one step further and use something like `void __name##` which
would make the symbol not even in the "namespace" (or prefix it in its own
namespace like "tor_free__*").
Anyway, we don't have to change the whole patch again but I think it would
1) help and simplify the code base especially helping developers creating
free function by using the DECL/IMPL macros and 2) hide the "private but
not really private" real free function so we never make the mistake of
using it directly.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/24337#comment:22>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs