[WIP] Applying modernize-use-override #6007
Conversation
|
Can one of the admins verify this patch? |
|
On the formatting: I tried to clang-format after running clang-tidy, but it seemed that ROOT is not properly formatted itself. There have been too many whitespace changes. So I did not run clang-format anywhere. |
|
Try |
|
I just pasted that in powershell thinking that surly that would not work on Windows. Surprisingly it did! :D |
bernhardmgruber
requested review from
agheata,
Axel-Naumann,
bellenot,
couet,
etejedor,
linev,
lmoneta,
osschar and
vgvassilev
as code owners
bernhardmgruber
changed the title
|
Further, some sub-projects use different formatting, so a global clang-format is maybe not the right thing to do. I personally have the IDE fix only the lines that have been worked on, so it gets the job done more slowly. |
The symlinks are already part of a different PR, but this PR depends on that one. I will rebase it away once the referred PR is accepted.
I used the .clang-format files within the ROOT working copy. My IDE should have formatted only a few lines where I manually changed the |
|
👍 for the links, I guess. There's a few headers that are getting generated, are these working OK? It's actually less file than I thought which have been touched. My rebasing would probably not be impacted by this. |
|
Wow, love this! Random unsolicited feedback: ROOT convention for commit messages is to use the imperative mood and start with a capital letter (we are not alone), and it might be useful to split the changes in different commits per repo subdirectories (tree, roofit, hist, math...) in case parts of this need to be reverted later for whatever reason. |
|
clang-format is "wrong" for ROOT header files. It does not support the align the function member name that we have been using since the start of the project. And indeed large "white space" only changes should be in their own PR. In this case, this means that (unfortunately) the work to add override includes not only "remove virtual, add override" but also add (by hand) white space to replace the virtual ('by hand' because the virtual is before the return type and the new space need to be after the return type). I/We appreciate your PR and your help with that :) |
|
Could you check |
Apply clang-tidy modernize-use-override check. Replace ClassDef by ClassDefOverride where VC complained while building Tree.
bernhardmgruber
force-pushed
the
override
branch
from
e08c394 to
3bde2a5
Compare
bernhardmgruber
force-pushed
the
override
branch
from
3bde2a5 to
b802615
Compare
I temporarily turned on declaration alignment and ran clang-format. Unfortunately, member functions are also not perfectly aligned in ROOT and that confuses clang-format. If aligning member function names is a desire, I suggest to consider trailing return types. It's beautiful! And you there is also a clang-tidy check to rewrite all signatures. |
|
I removed the dependence on the symlink PR because there seems to be more to it. That should make this PR lighter. |
I think this becomes obsolete now that I dropped the symlinks. |
| TObject *After(const TObject *obj) const; | ||
| TObject *First() const; | ||
| TObject *Last() const; | ||
| TObject * Before(const TObject *obj) const override; |
There was a problem hiding this comment.
If I recall correctly the coding convention says:
| TObject * Before(const TObject *obj) const override; | |
| TObject *Before(const TObject *obj) const override; |
There was a problem hiding this comment.
Probably, I do not remember it by heart. That's what clang-format should be for, so my IDE just formats while typing and I do not need to worry about anything.
Addressing the issue at hand, it seems like combining
AlignConsecutiveDeclarations: true
PointerAlignment: Right
does not give the expected results. It seems like clang-format needs a fix here.
There was a problem hiding this comment.
I gave it a quick try, but it seems that issue is more complicated to fix in clang-format. There is even a FIXME in the code stating:
// FIXME: Currently we don't handle properly the PointerAlignment: Right
// The * and & are not aligned and are left dangling. Something has to be done
// about it, but it raises the question of alignment of code like:
// const char* const* v1;
// float const* v2;
// SomeVeryLongType const& v3;
There was a problem hiding this comment.
clang-format cannot be configured to replay ROOT's old coding style. The new one is supported (but is waiting for #2298 to become documented...)
|
@phsft-bot build |
|
Starting build on |
|
My guess: In general I'm not sure how much we actually benefit from dressing this old code with some new clothes, also given the extensiveness of this change, which will create conflicts when backporting changes across this commit. Could we have a discussion on the benefit/cost ratio? @bernhardmgruber - what's your arguments? |
|
Build failed on ROOT-fedora31/noimt. Errors:
Warnings:
And 390 more |
|
Build failed on ROOT-fedora30/cxx14. Errors:
And 3 more Warnings:
And 430 more |
|
Build failed on mac1015/cxx17. Errors:
Warnings:
And 2547 more |
|
Build failed on windows10/cxx14. |
|
Build failed on ROOT-performance-centos7-multicore/default. Errors:
Warnings:
And 388 more |
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
Warnings:
And 390 more |
|
Build failed on mac1014/python3. Errors:
And 10 more Warnings:
And 2418 more |
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
And 10 more Warnings:
And 558 more |
|
We cannot merge this as is. @bernhardmgruber what do you want to do with this? Can we close this, discuss, and then do what we converged on? I don't think this "monster PR" :-) will survive without conflicts for long, and it still needs a ton of work. |
AFAIK I tried to apply this already in many places.
Well, that raises an even more general question: if there is little commitment to touch/refactor/maintain old code, what is your long term strategy for these parts of ROOT then? Will these old codes be deprecated/removed then? Code rot is a fact and it gets only worse over time. And so far I have the feeling ROOT will still be there in the next decade or the one after. And if these codes stay around than they should be maintained and improved occasionally. Of course maintenance takes resources. And I know that nobody ever has time to do it. But leaving everything as is in the face of possible improvements also has a cost that we pay be doing nothing. For this particular PR, if I jump into a header file, I do not know which methods are overriding something from a base class. But this knowledge helps me when reading new code. Now I pay the cost for having to look this information up in the inheritance hierarchy. In recent years we are lucky enough to even have automatic refactoring tools. They are far from perfect. But they are good. And applying them has very little cost for a moderate benefit. So I think at least those automatic refactorings should be applied to old code. Regular manual refactoring of production code would be even better, but I know I live in a dream world here ;) There is also the broken window theory, stating that code with bad quality encourages people working on it to also tolerate new code to be bad. I just started here in ROOT and I already heard from a few people that ROOT code is bad and I should have low expectations. So if we would improve the old code just a little we could also improve this mindset by a bit 👍 So please consider to apply at least automatic refactorings!
@hageboeck mentioned somewhere to me that he would prefer to have smaller PRs just targeting subparts of ROOT, even if the changes are simple/automatic. I guess I could break up the PR into smaller pieces. And maybe just focus on the parts I am involved with the most. However, I would like to see clang-tidy checks enabled on the entire code at some point. So we can raise the quality bar just a little bit more. |
I assume that the cost of backporting is not too high. @Axel-Naumann? Maybe also something to be discussed in the team meeting. What I would do personally, though, is to leave formatting out of the game. I set up my IDE to do formatting on lines I work on, but it leaves the rest untouched. That improves diffs a lot. Just imagine what somebody would see who wants to diff last Fridays ROOT against today's. |
|
Let's discuss on Monday during the team meeting. Would be great if you can make it, @bernhardmgruber ! |
|
In today's team meeting we discussed this PR and reached the following conclusion:
|
Uh oh!
There was an error while loading. Please reload this page.