On Sat, 5 Sep 2020 at 08:36, Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > Something based on the v4 patch makes sense.
Thanks for having a look at this. > I would add DEBUG1 back into the conditional, like > > if (__builtin_constant_p(elevel) && ((elevel) >= ERROR || (elevel) <= > DEBUG1) ? \ hmm, but surely we don't want to move all code that's in the same branch as an elog(DEBUG1) call into a cold area. With elog(ERROR) we generally have the form: if (<some condition we hope never to see>) elog(ERROR, "something bad happened"); In this case, we'd quite like for the compiler to put code relating to the elog in some cold area of the binary. With DEBUG we often have the form: <do normal stuff> elog(DEBUG1, "some interesting information"); <do normal stuff> I don't think we'd want to move the <do normal stuff> into a cold area. The v3 patch just put an unlikely() around the errstart() call if the level was <= DEBUG1. That just to move the code that's inside the if (errstart(...)) in the macro into a cold area. > Also, for the __has_attribute handling, I'd prefer the style that Andres > illustrated earlier, using: > > #ifndef __has_attribute > #define __has_attribute(attribute) 0 > #endif Yip, for sure. That way is much nicer. David
