On 11.01.2013 23:49, Tom Lane wrote:
> Andres Freund<andres@2ndquadrant.com> writes:
>> On 2013-01-11 16:28:13 -0500, Tom Lane wrote:
>>> I'm not very satisfied with that answer. Right now, Peter's
>>> patch has added a class of bugs that never existed before 9.3, and yours
>>> would add more. It might well be that those classes are empty ... but
>>> *we can't know that*. I don't think that keeping a new optimization for
>>> non-gcc compilers is worth that risk. Postgres is already full of
>>> gcc-only optimizations, anyway.
>
>> ISTM that ereport() already has so strange behaviour wrt evaluation of
>> arguments (i.e doing it only when the message is really logged) that its
>> doesn't seem to add a real additional risk.
>
> Hm ... well, that's a point. I may be putting too much weight on the
> fact that any such bug for elevel would be a new one that never existed
> before. What do others think?
It sure would be nice to avoid multiple evaluation. At least in xlog.c
we use emode_for_corrupt_record() to pass the elevel, and it does have a
side-effect. It's quite harmless in practice, you'll get some extra
DEBUG1 messages in the log, but still.
Here's one more construct to consider:
#define ereport_domain(elevel, domain, rest) \do { \ const int elevel_ = elevel; \ if
(errstart(elevel_,__FILE__,__LINE__, PG_FUNCNAME_MACRO, domain) ||
elevel_>= ERROR) \ { \ (void) rest; \ if (elevel_>= ERROR) \ errfinish_noreturn(1); \
else \ errfinish(1); \ } \} while(0)
extern void errfinish(int dummy,...);
extern void errfinish_noreturn(int dummy,...) __attribute__((noreturn));
With do-while, ereport() would no longer be an expression. There only
place where that's a problem in our codebase is in scan.l, bootscanner.l
and repl_scanner.l, which do this:
#undef fprintf
#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s",
msg)))
and flex does this:
(void) fprintf( stderr, "%s\n", msg );
but that's easy to work around by creating a wrapper fprintf function in
those .l files.
When I compile with gcc -O0, I get one warning with this:
datetime.c: In function ‘DateTimeParseError’:
datetime.c:3575:1: warning: ‘noreturn’ function does return [enabled by
default]
That suggests that the compiler didn't correctly deduce that
ereport(ERROR, ...) doesn't return. With -O1, the warning goes away.
- Heikki
