[systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set

[Djalal Harouni]

Hi Shawn,

On Tue, Mar 31, 2015 at 04:59:29PM -0700, Shawn Landden wrote:
> On Tue, Mar 31, 2015 at 2:40 PM, Djalal Harouni <tixxdz at opendz.org> wrote:
> > On Tue, Mar 31, 2015 at 11:10:34AM -0700, Shawn Landden wrote:
[...]
> >> The point is that assert() and assert_se() should only be used for
> >> programming errors,
> >> this is covered in CODING_STYLE.
> > No, I don't see any mention of assert_se().
> >
> >
> > With this you are breaking the logic that commes with the following
> > commits:
> > 0c0cdb06c139b52ff1
> > 787784c4c1b24a132
> > e80afdb3e4a123
> > 288c0991d5a0b240
> > b680a194bf9ed4c6
> > bdf7026e9557349c
> > 8d95631ea6c039a60b
> >
> > And the list is long... Please, check the code and the commit logs...
> >
> > You are also hiding the error logs. A simple "grep -r "assert_se" src/"
> > will show you perhaps the ones that can be improved without affecting
> > the semantics of other callers.
> >
> > You do realize that some of these are tests to make sure that systemd
> > works fine ? please read the commit logs, you will clearly see that the
> > authors do *not* want assert_se() to be optimized, and some authors will
> > even use assert_se() to handle errors in the core code.
> >
> > IOW this patch will *weaken* all the test infrastructure and perhaps
> > other areas.
> >
> > How tests are supposed to log errors after this ?
> >
> >
> >> I submitted a new version of this patch, with numbers (10kB smaller)
> >> to the mailing list.
> > Hmm thanks for the patch, this seems a valid number, but as I have said
> > I still think this is not the best approach.
> >
> > You are affecting all the callers, if you want to do this then update
> > the parts that really need to be updated, the real callers, IMHO before
> > you update a function, first check callers.
> >
> > So here if you think that the caller worth it and it is robust, then yeh
> > remove the assert_se() from there and make it handle the error correctly,
> > hmm but perhaps others may think differently...
> >
> > I will let others comment.
> Well the patch I last submitted still calls abort() (did you see that
> patch?). but yeah I
> don't have strong opinions about this....
Yes I saw the patch and the numbers, thank you!

I just didn't comment to let the thread clean, and see what others may
think.

Thanks!

-- 
Djalal Harouni
http://opendz.org