On Tue, Jan 17, 2012 at 1:23 PM, Noah Misch <noah@leadboat.com> wrote:
> On Mon, Jan 16, 2012 at 08:54:53PM -0500, Robert Haas wrote:
>> On Sun, Dec 18, 2011 at 11:53 AM, Noah Misch <noah@leadboat.com> wrote:
>> > Here's a version that calls sigsetjmp() once per file. ?While postgres=
ql.conf
>> > scanning hardly seems like the place to be shaving cycles, this does c=
atch
>> > fatal errors in functions other than yylex(), notably yy_create_buffer=
().
>>
>> This strikes me as more clever than necessary:
>>
>> #define fprintf(file, fmt, msg) \
>> =A0 =A0 0; /* eat cast to void */ \
>> =A0 =A0 GUC_flex_fatal_errmsg =3D msg; \
>> =A0 =A0 siglongjmp(*GUC_flex_fatal_jmp, 1)
>>
>> Can't we just define a function jumpoutoftheparser() here and call
>> that function rather than doing that /* eat cast to void */ hack?
>> That would also involve fewer assumptions about the coding style
>> emitted by flex. =A0For example, if flex chose to do something like:
>>
>> =A0 if (bumpity) fprintf(__FILE__, "%s", "dinglewump");
>>
>> ...the proposed definition would be a disaster. =A0I doubt that inlining
>> is a material performance benefit here; siglongjmp() can't be all that
>> cheap, and the error path shouldn't be all that frequent.
>>
>> Instead of making ParseConfigFp responsible for restoring
>> GUC_flex_fatal_jmp after calling anything that might recursively call
>> ParseConfigFp, I think it would make more sense to define it to stash
>> away the previous value and restore it on exit. =A0That way it wouldn't
>> need to know which of the things that it calls might in turn
>> recursively call it, which seems likely to reduce the chances of
>> present or future bugs. =A0A few comments about whichever way we go with
>> it seem like a good idea, too.
>
> Agreed on all points. =A0I also changed the save/restore of ConfigFileLin=
eno to
> work the same way. =A0New version attached.
OK, committed.
--=20
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
