root
root copied to clipboard
root-project
[ntuple, tidy] First attempt at a default set of checks and some fixes
This Pull request:
A first attempt at picking a good set of defaults for clang-tidy, together with the first 100 or so fixes in the RNTuple code base. It will be some effort to clean even just the RNTuple module, so I expect the cleanup to take several PRs.
Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds
Build failed on ROOT-debian10-i386/cxx14. Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build See console output.
Errors:
- [2021-07-29T15:55:46.465Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory
Failing tests:
Build failed on mac11.0/cxx17. Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build See console output.
Failing tests:
Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds
Build failed on ROOT-debian10-i386/cxx14. Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build See console output.
Errors:
- [2021-07-30T10:40:25.885Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory
Failing tests:
Hi Jakob, I don't know much about clang-tidy, why this specific set of checks?
I tried this clang-tidy configuration on the RDF code. Compared to the default configuration, which gives me almost zero errors, this conf produces 21k lines of errors.
There are a lot of errors that come from expanded gtest macros:
/home/blue/ROOT/master/tree/dataframe/test/dataframe_helpers.cxx:409:1: error: initializing non-owner argument of type 'testing::internal::TestFactoryBase *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory,-warnings-as-errors]
TEST(RunGraphs, EmptyListOfHandles)
^
/home/blue/ROOT/master/_build/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:2338:42: note: expanded from macro 'TEST'
#define TEST(test_suite_name, test_name) GTEST_TEST(test_suite_name, test_name)
^
/home/blue/ROOT/master/_build/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:2332:3: note: expanded from macro 'GTEST_TEST'
GTEST_TEST_(test_suite_name, test_name, ::testing::Test, \
^
/home/blue/ROOT/master/_build/googletest-prefix/src/googletest/googletest/include/gtest/internal/gtest-internal.h:1376:11: note: expanded from macro 'GTEST_TEST_'
new ::testing::internal::TestFactoryImpl<GTEST_TEST_CLASS_NAME_( \
^
/home/blue/ROOT/master/tree/dataframe/test/dataframe_helpers.cxx:409:1: error: class 'RunGraphs_EmptyListOfHandles_Test' defines a copy constructor and a copy assignment operator but does not define a destructor, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions,-warnings-as-errors]
TEST(RunGraphs, EmptyListOfHandles)
^
/home/blue/ROOT/master/_build/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:2338:42: note: expanded from macro 'TEST'
#define TEST(test_suite_name, test_name) GTEST_TEST(test_suite_name, test_name)
^
/home/blue/ROOT/master/_build/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:2332:3: note: expanded from macro 'GTEST_TEST'
GTEST_TEST_(test_suite_name, test_name, ::testing::Test, \
^
/home/blue/ROOT/master/_build/googletest-prefix/src/googletest/googletest/include/gtest/internal/gtest-internal.h:1355:9: note: expanded from macro 'GTEST_TEST_'
class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) \
^
/home/blue/ROOT/master/_build/googletest-prefix/src/googletest/googletest/include/gtest/internal/gtest-internal.h:1347:3: note: expanded from macro 'GTEST_TEST_CLASS_NAME_'
test_suite_name##_##test_name##_Test
^
note: expanded from here
/home/blue/ROOT/master/tree/dataframe/test/dataframe_helpers.cxx:418:1: error: variable 'test_info_' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables,-warnings-as-errors]
TEST(RunGraphs, AlreadyRun)
^
/home/blue/ROOT/master/_build/googletest-prefix/src/googletest/googletest/include/gtest/gtest.h:2338:42: note: expanded from macro 'TEST'
#define TEST(test_suite_name, test_name) GTEST_TEST(test_suite_name, test_name)
^
What can we do to silence these? Maybe running on tests needs a slightly different conf? Or do we just not want to run on directories containing tests?
My command:
clang-tidy (fd --type file --extension 'cxx' --extension 'hxx' . tree/dataframe/)
The output: clang-tidy.log
@eguiraud Thanks for taking a look! There is no specific reason for these checks. I went through the list and tried to select what makes sense. Please suggest changes to the list as you see fit. As for the tests: it would be nice to exclude the Google stuff from clang-tidy but keep our code under scrutiny. Not sure if this can be done though.
Alright! One other noisy check is the one for magic numbers: it complains about data member initialization such as fNBins = 64, which would not really be improved by a switch to constexpr static kDefNBins = 64; fNBins = kDefNBins. I would propose to turn that one off?
@jblomer you can manually disable warnings in some specific points of the code using a "NOLINT(error-type)" comment, see https://github.com/root-project/root/issues/7412#issuecomment-794182724 without having to turn them off globally.
Also, one suggestion, with QtCreator you can automatically fix all clang-tidy warnings using a button (at least for those that are easy to solve like NULL --> nullptr). See related pulls: https://github.com/root-project/root/pull/8045 https://github.com/root-project/root/pull/8144 https://github.com/root-project/root/pull/8265/ https://github.com/root-project/root/pull/8266/
Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds
Build failed on ROOT-debian10-i386/cxx14. Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build See console output.
Errors:
- [2021-08-05T08:19:30.444Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory
Failing tests:
About
it would be nice to exclude the Google stuff from clang-tidy but keep our code under scrutiny. Not sure if this can be done though.
I think at least in some cases clang-tidy can only complain about problems in template code if it sees the template instantiations in the tests, imho that's a strong argument for running clang-tidy on the tests too -- but of course we don't care about issues in googletest's macros...
Another one that we can't have ROOT-wide is modernize-concat-nested-namespaces
This one instead I guess is arguable, I don't mind fixing the (many) occurrences in tree/dataframe but I'd say it only makes sense if we make it part of ROOT coding conventions:
defines a copy constructor and a move constructor but does not define a destructor, a copy assignment operator or a move assignment operator [cppcoreguidelines-special-member-functions
Similarly for cppcoreguidelines-owning-memory: clang-tidy asks me to mark owning raw pointers with gsl::owner<>, but unless we allow its usage in ROOT's code base we will have many legitimate usages of owning raw pointers (e.g. to pass objects to jitted code) that will require a // NOLINT
Just fyi, Clang supports now multi-line NOLINTs: https://llvm.discourse.group/t/clang-tidy-multi-line-nolint/3041
Hi @jblomer, is this PR still relevant or can it be closed?
Maybe one thing to consider for the future: if you want to have a clang-tidy configuration specific to RNtuple, you can also put it in the right subdirectory, like I did here with RooFit:
https://github.com/root-project/root/blob/master/roofit/.clang-tidy
