On Mon Jul 25 15:31:45 2022 +0000, Piotr Caban wrote:
Have you considered something like in attached patch instead of introducing %S format? I think it's much more robust and I hope it will be possible to remove all hacks/special handling this way (probably except of end of templates case (> >)). The general idea is to introduce OPTIONAL_WS that is used to insert space in all the places where it's optional. The space is added only if first string ends with OPTIONAL_WS and second starts with OPTIONAL_WS. The attached patch was not really tested, it's just a quick WIP version to demonstrate the idea. [tmp.diff](/uploads/399b0a804f25b1c9adb6e820ba309429/tmp.diff) Hi Piotr,
the good side: it seems to be a step into the right direction (after adding a couple of missed items - not in wine regression list, though, --they should be added--), this seems to output things fine so perhaps, we can avoid adding ws only discrepancies in regression testing (*) what it shows (compared to current implementation): - current implement relies on string concatenation and hard coding spaces (with a couple of issues, and trying to patch things) - introducing a state (pending WS '\1') in the output buffer seems an interesting idea and helps contextualize when ws are needed or not *BUT* there are a bunch of things I don't like in the solution: - inserting optional WS both on LHS and RHS of a same word doesn't seem right - it looks like the dual LHS/RHS have been inserted to handle the '*' case (with either a space before in most of the cases, and sometimes not) - and even defining a keyword with OPTIONAL_WS doesn't seem right (whatever the side) my gut feeling is that native code looks similar to (in C++) ``` std:stringstream sstr; if (...) sstr << "const" << undname::ws; else if (...) sstr << "volatile" << undname::ws; else if (...) sstr << "const volatile" << undname::ws; // if (...) sstr << "*" << undname::ws; ``` where undname::ws should an iomanip for handling a pending white space separator in other words: - the control of ws should be attached to stream output/handling functions, not the keyword them selves - it requires to store a pending ws state to be resolved later on, depending on incoming output here's an attempt to get closer to the above: - derived from your proposal - it only uses RHS optional white spaces - not all possible cleanups have been done - still WIP at this stage - using a trailing '\1' to mark the str_print internal buffer with potential trailing space, and next inserted character is used to decide how to handle the '\1' [err](/uploads/f1bc1e97dec61387a09ea216d641011f/err) I'm still unhappy with - the new operator %t to cancel pending ws => still have to test whether moving the optional ws inserting to callers solve the issue so I'd rather go for a solution where: - '\1' trailing in strings (internal to str_printf) is here to mark optinal WS - callers should use (say '%w') to insert an optional ws comments welcomed (*) this one shows also bogus output (note the ' ' after the const and before the ',') (and I have other template examples with 'T const' parameters not generating the extra space... sigh) ?meth1(a)ZZZ@?$meth2(a)V?$YYY(a)V?$WWW(a)DU?$XXXs(a)D@RRR@@V?$UUU(a)D@2@@RRR@@PAVVVV(a)TTT@@U?$less(a)V?$WWW(a)DU?$XXX(a)D@RRR@@V?$UUU(a)D@2@@RRR@@@2(a)V?$UUU(a)U?$pair@$$CBV?$WWW(a)DU?$XXX(a)D@RRR@@V?$UUU(a)D@2@@std@@PAVVVV(a)TTT@@@RRR@@@2@$0A@@RRR@@@RRR@@QAEXXZ public: void __thiscall RRR::meth2<class RRR::YYY<class RRR::WWW<char,struct RRR::XXXs<char>,class RRR::UUU<char> >,class TTT::VVV *,struct RRR::less<class RRR::WWW<char,struct sRRR::XXX<char>,class RRR::UUU<char> > >,class RRR::UUU<struct RRR::pair<class std::WWW<char,struct RRR::XXX<char>,class RRR::UUU<char> > const ,class TTT::VVV *> >,0> >::ZZZ::meth1(void)" -- https://gitlab.winehq.org/wine/wine/-/merge_requests/492#note_4866