July 26, 2022
6:26 a.m.
On Mon Jul 25 15:31:45 2022 +0000, eric pouech wrote: > 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)" I generally like your idea but I have few comments: - I also don't like the %t format, I think it would be best if you find a way to avoid it (I see it as (maybe) useful way of adding hacks that would be nice to avoid) - I don't like handle_trailing_ws helper design, I don't think the decision should be made basing on the character value (it adds some hidden restrictions about str_printf usage e.g. in case of differentiating between operator* and pointer) Maybe the code can be changed in a way that "optinal white space" affects only %s parameter instead (so it's closer to your initial patch but adds trailing WS instead of leading WS and preserves the OPTIONAL_WS tag between str_printf calls)? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/492#note_4966