On Thu, Mar 26, 2015 at 5:47 PM, Djalal Harouni <tix...@opendz.org> wrote:
> On Fri, Mar 27, 2015 at 12:30:53AM +0100, Tom Gundersen wrote:
>> On Thu, Mar 26, 2015 at 9:19 AM, Lennart Poettering
>> <lenn...@poettering.net> wrote:
>> > On Tue, 24.03.15 11:11, Shawn Landden (sh...@churchofgit.com) wrote:
>> >
>> >> Will result in slightly smaller binaries, and cuts out the branch, even if
>> >> the expression is still executed.
>> >
>> > I am sorry, but the whole point of assert_se() is that it has side
>> > effects. That's why it is called that way (the "se" suffix stands for
>> > "side effects"), and is different from assert(). You are supposed to
>> > use it whenever the code enclosed in it shall not be suppressed if
>> > NDEBUG is defined. This patch of yours breaks that whole logic!
>>
>> Hm, am I reading the patch wrong? It looks good to me. It is simply
>> dropping the branching and logging in the NDEBUG case, but the
>> expression itself is still evaluated.
> Yep but it seems that abort() will never be called,
> log_assert_failed() => abort()
>
> And the logging mechanism is also one of those side effects. IMO unless
> there are real valid reasons for any change to these macors... changing
> anything here will just bring more bugs that we may never notice.
>
Those are the side effects of assert(), the side effects of the
expression are still evaluated.
>
>> So in the NDEBUG case "assert_se(foo())" becomes equivalent to
>> "foo()", which I guess makes sense (though I doubt it makes much of a
>> difference).
>>
>> -t
>>
>> >> ---
>> >> src/shared/macro.h | 12 ++++++------
>> >> 1 file changed, 6 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/src/shared/macro.h b/src/shared/macro.h
>> >> index 7f89951..02219ea 100644
>> >> --- a/src/shared/macro.h
>> >> +++ b/src/shared/macro.h
>> >> @@ -212,17 +212,17 @@ static inline unsigned long ALIGN_POWER2(unsigned
>> >> long u) {
>> >> (__x / __y + !!(__x % __y)); \
>> >> })
>> >>
>> >> -#define assert_se(expr) \
>> >> - do { \
>> >> - if (_unlikely_(!(expr))) \
>> >> - log_assert_failed(#expr, __FILE__, __LINE__,
>> >> __PRETTY_FUNCTION__); \
>> >> - } while (false) \
>> >> -
>> >> /* We override the glibc assert() here. */
>> >> #undef assert
>> >> #ifdef NDEBUG
>> >> +#define assert_se(expr) do {expr} while(false)
>> >> #define assert(expr) do {} while(false)
>> >> #else
>> >> +#define assert_se(expr) \
>> >> + do { \
>> >> + if (_unlikely_(!(expr))) \
>> >> + log_assert_failed(#expr, __FILE__, __LINE__,
>> >> __PRETTY_FUNCTION__); \
>> >> + } while (false)
>> >> #define assert(expr) assert_se(expr)
>> >> #endif
>> >>
>> >> --
>> >> 2.2.1.209.g41e5f3a
>> >>
>> >> _______________________________________________
>> >> systemd-devel mailing list
>> >> systemd-devel@lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>> >
>> >
>> > Lennart
>> >
>> > --
>> > Lennart Poettering, Red Hat
>> > _______________________________________________
>> > systemd-devel mailing list
>> > systemd-devel@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>> _______________________________________________
>> systemd-devel mailing list
>> systemd-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
> --
> Djalal Harouni
> http://opendz.org
> _______________________________________________
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
Shawn Landden
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
