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@ZZZ@?$meth2@V?$YYY@V?$WWW@DU?$XXXs@D@RRR@@V?$UUU@D@2@@RRR@@PAVVVV@TTT@@U?$less@V?$WWW@DU?$XXX@D@RRR@@V?$UUU@D@2@@RRR@@@2@V?$UUU@U?$pair@$$CBV?$WWW@DU?$XXX@D@RRR@@V?$UUU@D@2@@std@@PAVVVV@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)?