[tor-bugs] #24337 [Core Tor/Tor]: Every _free() function should be a macro that sets the corresponding pointer to NULL.

[Tor Bug Tracker & Wiki]

#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