Re: replacing OUString::valueOf(static_cast<sal_Int32>) ??

On Thursday 10 of January 2013, Noel Grandin wrote:
> On 2013-01-10 15:55, Lubos Lunak wrote:
> > - There's no need for valueOfChar(). There is already OUString ctor from
> > sal_Unicode, so the valueOf() overload for it is just making an obvious
> > thing complicated. Code using it can be converted to use the ctor
> > instead.
>
> Which doesn't help with overload resolution problems.

 It does. If you say it's wrong to use valueOfWhatever() for char->string 
conversion, there either will not be problems, or it will be wrong. If you 
want to fix an existing problem by introducing a new function, how does that 
make anything better than using an already existing function that does the 
job?

> > - When more or less deprecating valueOf() this way, it has also float
> > overloads, so something should be created for those too.
>
> If those also suffer from overload resolution problems, then sure.
> But
>       git grep "String::valueOf.*static_cast<.*double"
> doesn't find anything that looks like it needs help.
>
> > - I'm still not sold on the naming, OUString::valueInt() doesn't say much
> > and OUString::valueOfInt() feels cryptic. Can we please use something
> > obvious that doesn't need decyphering, such as OUString::number() or
> > OUString::fromInt() (as much as I still don't like the idea of harcoding
> > the irrelevant type information in the name)?
>
> Which would be inconsistent with the existing method names.

 Oh. I think we may be talking past each others because we have different 
goals. Am I correct in the assumption that you merely want to add another set 
of functions that people can use whenever the originals don't work?

 I do not think it is a good idea to just do a quick hack to paper over a 
problem. The strings are one of the most basic classes in the LO code, and if 
we are going to do changes there, we may as well try to do them properly. 
Base classes are not something we should randomly throw changes at, and we 
can still spend ages fixing up all the design decisions in the string classes 
that already are there and could have been done better. So while I appreciate 
it that you're willing to write a kludge that'll help avoid a problem, I kind 
of consider it a waste of your time, when with just little more effort a 
proper solution could be created.

 If you just add another set of functions, it will be confusing which one to 
use. And people will still sometimes use the old one out of habbit or for 
whatever reason, and still the problem will occassionally show up, and then 
the code will need to be changed to the new function, which is not really 
that different to just adding a cast. And if you say that people should 
always directly use the new one, then you may as well remove the old one 
(except keep it for extensions backwards compatibility), and then all I've 
said applies (no need for char function, float function needed, use a good 
name, etc.).

 Or did I misunderstand the intent of your patch?

> >   I expect it won't, regular expressions can't tell what foo is in
> > "valueOf( foo )".
>
> Actually, regex can. It's called capturing groups.

 Capturing groups may tell you that foo is foo, but it won't tell you it's a 
float. So how will you know to which of your proposed valueX() functions you 
should change it?

> > Unless all you want to convert is only places which do the explicit
> > cast, this will need a (fairly simple) Clang plugin.
>
> Sure, if you feel like writing one.

 Actually, I'd prefer to write a howto about that first, whenever I get to 
doing that, so that I don't have to write every single plugin. Such a plugin 
will be still much simpler than a regexp or any other way.

-- 
 Lubos Lunak
 l.lunak@suse.cz