[Development] Reverting the QRegExp change?
Giuseppe D'Angelo
dangelog at gmail.com
Wed May 2 11:21:36 CEST 2012
On 2 May 2012 09:48, Thiago Macieira <thiago.macieira at intel.com> wrote:
> Given the negative reaction here and on Gerrit, I'm wondering if we should
> revert it.
>
> In fact, for the simple fact that the SIC change wasn't discussed here before
> it went in (my bad, sorry for that), it deserves to be reverted.
>
> What do you think?
That two kind of assestments for end-user code must be done:
1) checking the actual breakages (f.i. compiling Creator or KDE are
good testcases)
2) checking if it's worth it to spend N days of work (Brad said he
spent a full day to fix Creator) going around and replacing every
rx.indexIn(...) with sth like QRegExp copy(rx); copy.indexIn()
> Another option is what Olivier has proposed:
> https://codereview.qt-project.org/#change,24986
Adding the const overloads to avoid breaking the source is a good
idea. I'm not too happy with *that* implementation... the point of
your change was
1) that a const method is not supposed to modify the object in any
"visible" way;
2) to avoid to break the (totally undocumented) assumption that
calling const methods on a reentrant class is actually thread safe;
well: now those const overloads are not doing any of 1+2, bringing us
to the situation before your change. So why dropping the const in the
first place?! :)
In a related patch I suggested something like
int QRegExp::foo() const
{
QRegExp copy = *this;
return copy.foo();
}
which although being source compatible, is behaviour incompatible
(QRegExp users do expect const methods to change the object -- it's
even documented!).
So it's a matter of deciding what's the least worst option.
--
Giuseppe D'Angelo
(For an extreme bikeshedding round, is anyone really relying on a
const QRegExp object to modify itself? I tend to believe that if one
is really interested in the captured texts, etc. then it's very likely
that she's already using a non-const object!)
More information about the Development
mailing list