> On 14 Mar 2024, at 09:06, Michael Paquier <mich...@paquier.xyz> wrote:
> I think that it is cleaner to create the new API first, and then > rely on it, reversing the order of both patches I agree with this ordering. > (perhaps the extra destroyStringInfo in freeJsonLexContext() should > have been moved in 0001). I wouldn't worry about that, seems fine as is to me. > See the attached with few tweaks to 0001, previously 0002 @-@. > I'd still need to do a more serious lookup of 0002, previously 0001. A few small comments: - * +* Whitespace + /* don't allow destroys of read-only StringInfos */ + Assert(str->maxlen != 0); Considering that StringInfo.c don't own the memory here I think it's warranted to turn this assert into an elog() to avoid the risk of use-after-free bugs. + * The returned allocation is either static or owned by the JsonLexContext and + * should not be freed. The most important part of that comment is at the very end, to help readers I would reword this to just "The returned pointer should not be freed.", or at least put that part first. +#define token_error(lex, format) \ I'm not sure this buys much more than reduced LoC, the expansion isn't unreadable to the point that the format constraint encoded in the macro is worth it IMO. -- Daniel Gustafsson