On 26.05.26 13:54, Ulf Hermann via Development wrote:
* As for refactorings: no, refactorings cannot have regressions. If a
change causes regressions, then by definition it wasn't a
refactoring. That the term is used to mean arbitrary rewrites
instead of actual refactorings is part of the problem, though. From
refactoring.com:
*Refactoring* is a disciplined technique for restructuring an
existing body of code, altering its internal structure without
changing its external behavior.
Its heart is a series of small behavior preserving transformations.
Apparently we are quite often doing changes we believe to be refactorings, but
that actually aren't. How do we deal with that?
This one is easy: It says it right there on the front page of
https://refactoring.com/: "Refactoring is a part of day-to-day programming"
section. In the book itself, he recommends the Two Hats: When you refactor,
never change the observable behaviour of your code. That's you with your
refactoring hat on. If you do make observable changes (to optimize, bugfix, or
implement a new feature), wear _that_ hat. Only change the behaviour, but not
the structure of the code. Always keep the two hats separate, but, in practice,
you exchange them every few minutes, but, crucially, separated by at least a
save-and-test, better by a commit (so reviewers can follow the changes as
easily). This is kinda actually codified in our Commit Policy Section 8.1,
though largely ignored by Qt devs, generally speaking.
You can see this in action in most of my commit chains: If I fix a bug, I first
try to create a reproducer in some tst_ class, and XFAIL that (doesn't work for
UB fixes, obviously). Then, in a separate commit, I apply the fix and remove
the EXPECT_FAIL. And both are integrated in _separate_ integration runs, and
the first one might get picked further back than the actual fix.
Or in the QArrayData case, I was also first refactoring to the point where the
fix was localized and trivial. Unfortunately, Gerrit destroyed the Relation
Chain in dev, but here's a screenshot of the one in 6.5, almost all ducks in a
row (before that, too, is destroyed by Gerrit):
[cid:b22a5eec-8bac-48d9-9780-9636e40b8fa5]
To reviewers who see this and go "squash this": _I_ need this structure, for
when I'm my own reviewer. So I won't quash, because this is not how I develop.
This is the result of local squashing and splitting and resorting (eternal
thanks to Dennis for introducing me to git-revise!) to make my large mess into
a coherent chain of commits for reviewers, incl. myself, to consume. It's me
seeing the fruit of my labour in a different light for the first time. Ideally,
I'd like each commit to be trivial, which, again, I require for myself, because
I'm often working in code that I'm not familiar with. Not always possible,
unfortunately.
And to TPTBs who see this and go "the fix should be first, then the
refactoring, and only the fix should be picked back": That's just not how it
works. Sometimes, it does, and then that's fair to ask, esp. in an area where
nothing gets picked back, usually, but in the QArrayData case, I first needed
to refactor before I could fix. For my own sanity and that of reviewers.
HTH,
Marc
--
Marc Mutz <[email protected]><mailto:[email protected]> (he/his)
Principal Software Engineer
The Qt Company
Erich-Thilo-Str. 10 12489
Berlin, Germany
www.qt.io<http://www.qt.io>
Geschäftsführer: Mika Pälsi, Juha Varelius, Juha Puputti
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht Charlottenburg,
HRB 144331 B
Confidential
--
Development mailing list
[email protected]
https://lists.qt-project.org/listinfo/development
