[Development] QString behavior change

[Thiago Macieira]

On Tuesday 11 August 2015 18:45:10 Julien Blanc wrote:
> Le mardi 11 août 2015 à 08:46 -0700, Thiago Macieira a écrit :
> > On Tuesday 11 August 2015 10:38:27 Julien Blanc wrote:
> > > That point is certainly valid, but i would like to raise the point that
> > > string nullness is a *required* feature for QtSQL (a null QString is
> > > converted to a NULL SQL, whereas a non-null empty QString is converted
> > 
> > That's a misfeature. QtSql should have been using something like
> > QOptional<QString> for a nullable string. In addition, it usually uses
> > QVariant, which has its own null setting -- it allows for a null int,
> > different from a value of zero.
> 
> What are the chances that such a change can be accepted ? (i mean, i can
> submit such a patch, but that would mean breaking a *lot* of code).

QOptional does not exist yet and for very good reasons.

QVariant is already in use, so I don't think you'd be sending a patch to use 
QVariant.

What are you proposing to send?

> > > So, could you consider returning StringType("") instead ? (unless i’m
> > > wrong, null strings should be handled by the first condition).
> > 
> > No.
> 
> By no, do you mean that i am wrong (ie, a null QString passing via
> trimmed_helper would fail the begin == str.cbegin() && end == str.cend()
> test, for a reason i don’t understand, nor does my compiler), or that
> you have a good reason for not returning StringType("") ? In the latter
> case, i would be very glad if you explain it.
> 
> > If you want to pass a null QString to QtSql, you can. If you want to check
> > whether a string you got from QtSql is null, you can.
> 
> Please consider the following case :
> 
> if(str.isNull())
>    q.bind(0, QString(""));
> else
>    q.bind(0, str.trimmed());
> 
> That code looks perfectly valid, but is in fact broken by a subtle,
> counter-intuitive implementation detail.

The code looks really weird. I'd just do:
	q.bind(0, notNull(str.trimmed()));

And add this notNull() function somewhere.

But you've violated what I said below by assuming trimmed() keeps nullness. 
Any function not specifically documented to deal with nulls should be expected 
to change. If it's not trimmed(), it could be something else.

In these, if str is null on input, will the output be null? What if it is 
empty-but-not-null on input, will it remain not null?

	QUrl(str).toString();
	QString::fromUtf8(str.toUtf8());
	QFile::decodeName(QFile::encodeName(str));
	str.replace("foo", "bar");
	str.toUpper();

There are just too many places where strings are mutated, so we cannot offer to 
keep the nullness stable for everything.

> > That has nothing to do with trimmed(). Other API doesn't guarantee to keep
> > null either when mutating strings, so I don't see why we should here. If
> > the API doesn't specifically deal with null QStrings, expect that it will
> > randomly change between null and not null.
> 
> If nullness of QString is so much broken, then please consider removing
> it at all (Qt6 ?). I will thank you for that. But until that’s done, is
> it possible to not add more brokenness to it ?

We should remove it, indeed, but only after std::optional can be used properly 
in API that needs it (like after refactoring QtSql). I don't think it will 
happen in time for Qt 6.

I don't consider this as "adding more brokenness".

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center