root [tree] Add missing `override` specifiers in tree/dataframe

trafficstars

For developers, it is unconvenient that the override specifier that flags member functions as overriding on first sight is not used so much in hist.

I think it is a good time to add the missing override specifiers to more ROOT components now, as I have already done it for roofit, math, and hist, where it was really a game changer for me because it made reading und understanding the code easier!

See:

https://github.com/root-project/root/pull/10083
https://github.com/root-project/root/pull/9808
https://github.com/root-project/root/pull/9883

This commit was generated more or less automatically with this Python script that uses clang-tidy:

import os
import glob
import subprocess
import tqdm

"""
For clang-tidy to work, you have to copy the compile_commands.json from the
build directory back into the repo directory (just like in
.ci/copy_headers.sh).
"""

def get_sources(directory, extensions):

    files = []

    for ext in extensions:
        files += glob.glob(
            os.path.join(directory, "**/*" + ext), recursive=True
        )

    return files

"""
Recursively find extensions in directory, to figure out whic hextensions
should be globbed for.
   find . -type f -name '*.*' | sed 's|.*\.||' | sort -u
"""
extensions = [".h", ".hpp", ".hxx", ".cpp", ".cc", ".cxx"]

"""
Some extensions are recognized as C and not as C++ files by clang-tidy. We
need to rename them, and this dict specifies how file extensions should be
replaced.
"""
rename_dict = {".h": ".hpp"}

files = get_sources("tree", extensions)

cflags = (
    subprocess.check_output(["root-config", "--cflags"]).strip().decode("utf-8")
)

for file in tqdm.tqdm(files):

    file_renamed = file
    for ext, ext_renamed in rename_dict.items():
        if file.endswith(ext):
            file_renamed = file.replace(ext, ext_renamed)

    if file_renamed != file:
        os.rename(file, file_renamed)

    cmd = [
        "clang-tidy",
        "-checks=modernize-use-override",
        "--fix",
        file_renamed,
        "--",
    ] + cflags.split(" ")
    subprocess.call(cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)

    if file_renamed != file:
        os.rename(file_renamed, file)

"""
Finally, replace the ClassDef with the ClassDefOverride macros.
  find tree -type f -print | xargs sed -i 's/ClassDef(/ClassDefOverride(/'
...and change back the ClassDefOverride of non-overriding base classes.
"""

Aug 30 '22 14:08 guitargeek

Hi!

most if not all override should actually be final

Are your referring to dataframe here or all the tree subsystem? And would this not break usercode where people are further inheriting from the overriding classes?

Aug 30 '22 14:08 guitargeek

Updated the PR with leaving lexertk.hpp alone and also adding missing overrides in compiler macros.

Aug 30 '22 15:08 guitargeek

Are your referring to dataframe here or all the tree subsystem?

dataframe

would this not break usercode where people are further inheriting from the overriding classes?

In RDF, the only public classes that are designed for inheritance are RActionImpl and RDataSource (they should not be final or have methods marked final, but they do not have a base class either so I don't think this PR touches them). All other classes are in namespace Detail or Internal and I cannot think of any case where I designed them for multiple levels of inheritance.

Aug 30 '22 15:08 eguiraud

Alright, I have reverted the addition of override to the destructors in dataframe and added final where I could. That was indeed almost everywhere, except for RRange::GetGraph().

Aug 30 '22 15:08 guitargeek

It might be useful to seperate this PR in 3 (RDataFrame, TTree and RNTuple). On first glance, the set of over-ride in TTree seems to be much lower in number that I would have expected.

Aug 30 '22 15:08 pcanal

Yes, I was also happy to see that it was not too many! Okay, I'll split them up

Aug 30 '22 15:08 guitargeek

Hi @eguiraud, I have implemented your suggestions and split of the non-RDF changes to other PRs.

Aug 31 '22 11:08 guitargeek

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14 How to customize builds

Sep 27 '22 10:09 phsft-bot

Build failed on windows10/cxx14. Running on null:C:\build\workspace\root-pullrequests-build See console output.

Errors:

[2022-09-27T11:13:12.554Z] C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\Vectorize\SLPVectorizer.cpp : fatal error C1085: Cannot write compiler generated file: 'C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\Vectorize\LLVMVectorize.dir\Release\SLPVectorizer.obj': Invalid argument [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\Vectorize\LLVMVectorize.vcxproj]
[2022-09-27T11:13:14.083Z] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\type_traits(579,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\Utils\Local.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\Utils\LLVMTransformUtils.vcxproj]
[2022-09-27T11:13:15.619Z] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\xutility(3822,12): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\IPO\InlineSimple.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\IPO\LLVMipo.vcxproj]
[2022-09-27T11:13:15.619Z] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\vector(1724,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\IPO\MergeFunctions.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\IPO\LLVMipo.vcxproj]
[2022-09-27T11:13:15.619Z] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\xtree(1933,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\Utils\Mem2Reg.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\Utils\LLVMTransformUtils.vcxproj]
[2022-09-27T11:13:15.619Z] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\utility(66,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\IPO\LoopExtractor.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\IPO\LLVMipo.vcxproj]
[2022-09-27T11:13:15.619Z] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\tuple(327,32): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\Utils\LCSSA.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\Utils\LLVMTransformUtils.vcxproj]
[2022-09-27T11:13:15.619Z] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\atomic(2140,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\Utils\LoopUnroll.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\Utils\LLVMTransformUtils.vcxproj]
[2022-09-27T11:13:16.354Z] C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(700,5): error MSB8071: Cannot parse tool output 'C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\include\llvm/Support/Endian.h(225,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\CodeGen\RegAllocBasic.cpp)' with regex '^In file included from .*$': Exception of type 'System.OutOfMemoryException' was thrown. [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\CodeGen\LLVMCodeGen.vcxproj]
[2022-09-27T11:13:16.354Z] C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\Utils\LowerInvoke.cpp(97,1): fatal error C1060: compiler is out of heap space [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\Utils\LLVMTransformUtils.vcxproj]

And 80 more

Sep 27 '22 11:09 phsft-bot

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14 How to customize builds

Oct 01 '22 16:10 phsft-bot

Hi @eguiraud, thanks for the review and the comments! I addressed almost all of them, except for the ones related to the destructors.

There are three options we have for the overriding destructors, and there are arguments against all of them:

Add override as suggested by clang-tidy, which you don't like because it appears redundant
Adding nothing, but then removing the base class would break downsteam inheritance
Adding virtual, but this implies that it's the first function in the override chain

Let me know what you want me to do! Personally, I'm for doing what clang-tidy wants :)

Clang tidy suggests to add override to the constructor if it overrides a base class,

Oct 01 '22 16:10 guitargeek

Build failed on ROOT-ubuntu2004/python3. Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build See console output.

Errors:

[2022-10-01T16:22:57.543Z] FAILED: tmva/sofie/test/CMakeFiles/SofieCompileModels_ROOT.util

Oct 01 '22 16:10 phsft-bot

Build failed on ROOT-ubuntu18.04/nortcxxmod. Running on sft-ubuntu-1804-2.cern.ch:/build/workspace/root-pullrequests-build See console output.

Failing tests:

And 512 more

Oct 01 '22 16:10 phsft-bot

Thank you for your patience @guitargeek :)

Given clang-tidy's behavior, I think you are right, option 1 is the best: I'd rather have clang-tidy-compliant code than code that looks exactly how I like it :)

Oct 03 '22 07:10 eguiraud

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14 How to customize builds

Oct 03 '22 12:10 phsft-bot

Build failed on ROOT-ubuntu2004/python3. Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build See console output.

Errors:

[2022-10-03T12:16:06.721Z] FAILED: lib/modules.idx

Oct 03 '22 12:10 phsft-bot

Build failed on ROOT-ubuntu18.04/nortcxxmod. Running on sft-ubuntu-1804-2.cern.ch:/build/workspace/root-pullrequests-build See console output.

Failing tests:

And 512 more

Oct 03 '22 12:10 phsft-bot

Build failed on windows10/cxx14. Running on null:C:\build\workspace\root-pullrequests-build See console output.

Failing tests:

Oct 03 '22 13:10 phsft-bot

Build failed on mac1015/cxx17. Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build See console output.

Failing tests:

Oct 03 '22 15:10 phsft-bot

root root copied to clipboard

[tree] Add missing `override` specifiers in tree/dataframe

Errors:

Errors:

Failing tests:

Errors:

Failing tests:

Failing tests:

Failing tests:

root
root copied to clipboard