Patrick Steinhardt <ps@xxxxxx> writes: > Good point. It does make sense for `_free()` functions to handle NULL > pointers, but doesn't quite for `_release()` ones. I agree that foo_free() should accept NULL and silently become a no-op. I do not care deeply whether foo_release() did the same, or not, as long as all *_release()s behave the same way. Maybe it is more convenient if they ignored NULL, as I have a hunch that feeding a NULL pointer to foo_release() is unlikely to be a bug. Since we documented our aspiration to use these (and foo_clear()) consistently, we may #leftoverbits want to also document the calling convention as well. >> And symdiff_prepare() at least clears its .skip member to NULL, so >> this pre-initialization is probably not needed. If we are preparing >> ourselves for future changes of the flow in this function (e.g. >> goto's that jump to the clean-up label from which symdiff_release() >> is always called, even when we did not call symdiff_prepare() on >> this thing), this is probably not sufficient to convey that >> intention (instead I'd use an explicit ".skip = NULL" to say "we >> might not even call _prepare() but this one is prepared to be passed >> to _release() even in such a case"). >> >> Given that there is no such goto exists, and that _prepare() always >> sets up the .skip member appropriately, I wonder if we are much >> better off leaving sdiff uninitialized at the declaration site here. >> If we add such a goto that bypasses _prepare() in the future, the >> compiler will notice that we are passing an uninitialized sdiff to >> _release(), no? > > You'd hope it does, but it certainly depends on your compiler flags. > Various hardening flags for example implicitly initialize variables, and > I have a feeling that this also causes them to not emit any warnings > anymore. At least I only spot such warnings in CI. Yeah, that is a sad fact in the real world X-<. To be defensive, I think an explicit "{ .skip = NULL }" or "{ 0 }" would not be too bad and may even serve as a good reminder for developers who may want to jump over the call to _prepare() in the future. The explicit ".skip = NULL" says "we know it is safe to call _release() with a struct that hasn't gone through _prepare(), as long as its .skip member is cleared", but the story "{ 0 }" tells us is not much more than "we clear just like everybody else", and that is why I suggested the former (iow, I know both mean the same thing to the C compiler---I just care more about what it tells the human readers). Thanks.