Hendrik/xgboost
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Update clang-tidy. (#10730)

Browse Source 
- Install cmake using pip.
- Fix compile command generation.
- Clean up the tidy script and remove the need to load the yaml file.
- Fix modernized type traits.
- Fix span class. Polymorphism support is dropped
This commit is contained in:
Jiaming Yuan
2024-08-22 04:12:18 +08:00
committed by GitHub
parent 03bd1183bc
commit cb54374550
34 changed files with 361 additions and 387 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
tests/ci_build/Dockerfile.clang_tidy
View File

@@ -11,20 +11,28 @@ RUN \
apt-get update && \
apt-get install -y wget git python3 python3-pip software-properties-common \
apt-transport-https ca-certificates gnupg-agent && \
apt-get install -y llvm-15 clang-tidy-15 clang-15 libomp-15-dev && \
apt-get install -y cmake
apt-get install -y ninja-build
# Install clang-tidy: https://apt.llvm.org/
RUN \
apt-add-repository "deb http://apt.llvm.org/jammy/ llvm-toolchain-jammy-19 main" && \
wget -O llvm-snapshot.gpg.key https://apt.llvm.org/llvm-snapshot.gpg.key && \
apt-key add ./llvm-snapshot.gpg.key && \
rm llvm-snapshot.gpg.key && \
apt-get update && \
apt-get install -y clang-tidy-19 clang-19 libomp-19-dev
# Set default clang-tidy version
RUN \
update-alternatives --install /usr/bin/clang-tidy clang-tidy /usr/bin/clang-tidy-15 100 && \
update-alternatives --install /usr/bin/clang clang /usr/bin/clang-15 100
update-alternatives --install /usr/bin/clang-tidy clang-tidy /usr/bin/clang-tidy-19 100 && \
update-alternatives --install /usr/bin/clang clang /usr/bin/clang-19 100
RUN \
apt-get install libgtest-dev libgmock-dev -y
# Install Python packages
RUN \
pip3 install pyyaml
pip3 install cmake
ENV GOSU_VERSION=1.10

3
tests/ci_build/lint_python.py
View File

@@ -16,6 +16,8 @@ class LintersPaths:
BLACK = (
# core
"python-package/",
# CI
"tests/ci_build/tidy.py",
# tests
"tests/python/test_config.py",
"tests/python/test_callback.py",
@@ -119,6 +121,7 @@ class LintersPaths:
"demo/guide-python/learning_to_rank.py",
"demo/aft_survival/aft_survival_viz_demo.py",
# CI
"tests/ci_build/tidy.py",
"tests/ci_build/lint_python.py",
"tests/ci_build/test_r_package.py",
"tests/ci_build/test_utils.py",

289
tests/ci_build/tidy.py
View File

@@ -1,4 +1,6 @@
#!/usr/bin/env python
from __future__ import annotations
import argparse
import json
import os
@@ -9,20 +11,17 @@ import sys
from multiprocessing import Pool, cpu_count
from time import time
import yaml
def call(args: list[str]) -> tuple[int, int, str, list[str]]:
"""Subprocess run wrapper."""
completed = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
error_msg = completed.stdout.decode("utf-8")
# `workspace` is a name used in the CI container. Normally we should keep the dir
# as `xgboost`.
matched = re.search(
"(workspace|xgboost)/.*(src|tests|include)/.*warning:", error_msg, re.MULTILINE
)
def call(args):
'''Subprocess run wrapper.'''
completed = subprocess.run(args,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
error_msg = completed.stdout.decode('utf-8')
# `workspace` is a name used in Jenkins CI. Normally we should keep the
# dir as `xgboost`.
matched = re.search('(workspace|xgboost)/.*(src|tests|include)/.*warning:',
error_msg,
re.MULTILINE)
if matched is None:
return_code = 0
else:
@@ -30,195 +29,203 @@ def call(args):
return (completed.returncode, return_code, error_msg, args)
class ClangTidy(object):
''' clang tidy wrapper.
class ClangTidy:
"""clang tidy wrapper.
Args:
args: Command line arguments.
cpp_lint: Run linter on C++ source code.
cuda_lint: Run linter on CUDA source code.
use_dmlc_gtest: Whether to use gtest bundled in dmlc-core.
'''
def __init__(self, args):
"""
def __init__(self, args: argparse.Namespace) -> None:
self.cpp_lint = args.cpp
self.cuda_lint = args.cuda
self.use_dmlc_gtest: bool = args.use_dmlc_gtest
self.cuda_archs = args.cuda_archs.copy() if args.cuda_archs else []
if args.tidy_version:
self.exe = 'clang-tidy-' + str(args.tidy_version)
self.exe = "clang-tidy-" + str(args.tidy_version)
else:
self.exe = 'clang-tidy'
self.exe = "clang-tidy"
print('Run linter on CUDA: ', self.cuda_lint)
print('Run linter on C++:', self.cpp_lint)
print('Use dmlc gtest:', self.use_dmlc_gtest)
print('CUDA archs:', ' '.join(self.cuda_archs))
print("Run linter on CUDA: ", self.cuda_lint)
print("Run linter on C++:", self.cpp_lint)
print("Use dmlc gtest:", self.use_dmlc_gtest)
print("CUDA archs:", " ".join(self.cuda_archs))
if not self.cpp_lint and not self.cuda_lint:
raise ValueError('Both --cpp and --cuda are set to 0.')
raise ValueError("Both --cpp and --cuda are set to 0.")
self.root_path = os.path.abspath(os.path.curdir)
print('Project root:', self.root_path)
self.cdb_path = os.path.join(self.root_path, 'cdb')
print("Project root:", self.root_path)
self.cdb_path = os.path.join(self.root_path, "cdb")
def __enter__(self):
def __enter__(self) -> "ClangTidy":
self.start = time()
if os.path.exists(self.cdb_path):
shutil.rmtree(self.cdb_path)
self._generate_cdb()
return self
def __exit__(self, *args):
def __exit__(self, *args: list) -> None:
if os.path.exists(self.cdb_path):
shutil.rmtree(self.cdb_path)
self.end = time()
print('Finish running clang-tidy:', self.end - self.start)
print("Finish running clang-tidy:", self.end - self.start)
def _generate_cdb(self):
'''Run CMake to generate compilation database.'''
def _generate_cdb(self) -> None:
"""Run CMake to generate compilation database."""
os.mkdir(self.cdb_path)
os.chdir(self.cdb_path)
cmake_args = ['cmake', '..', '-DCMAKE_EXPORT_COMPILE_COMMANDS=ON',
'-DGOOGLE_TEST=ON']
cmake_args = [
"cmake",
self.root_path,
"-GNinja", # prevents cmake from using --option-files for include path.
"-DCMAKE_EXPORT_COMPILE_COMMANDS=ON",
"-DGOOGLE_TEST=ON",
"-DCMAKE_CXX_FLAGS='-Wno-clang-diagnostic-deprecated-declarations'",
]
if self.use_dmlc_gtest:
cmake_args.append('-DUSE_DMLC_GTEST=ON')
cmake_args.append("-DUSE_DMLC_GTEST=ON")
else:
cmake_args.append('-DUSE_DMLC_GTEST=OFF')
cmake_args.append("-DUSE_DMLC_GTEST=OFF")
if self.cuda_lint:
cmake_args.extend(['-DUSE_CUDA=ON', '-DUSE_NCCL=ON'])
cmake_args.extend(["-DUSE_CUDA=ON", "-DUSE_NCCL=ON"])
if self.cuda_archs:
arch_list = ';'.join(self.cuda_archs)
cmake_args.append(f'-DGPU_COMPUTE_VER={arch_list}')
arch_list = ";".join(self.cuda_archs)
cmake_args.append(f"-DCMAKE_CUDA_ARCHITECTURES={arch_list}")
subprocess.run(cmake_args)
os.chdir(self.root_path)
def convert_nvcc_command_to_clang(self, command):
'''Convert nvcc flags to corresponding clang flags.'''
def convert_nvcc_command_to_clang(self, command: str) -> str:
"""Convert nvcc flags to corresponding clang flags."""
components = command.split()
compiler: str = components[0]
if compiler.find('nvcc') != -1:
compiler = 'clang++'
if compiler.find("nvcc") != -1:
compiler = "clang++"
components[0] = compiler
# check each component in a command
converted_components = [compiler]
for i in range(1, len(components)):
if components[i] == '-lineinfo':
if components[i] == "-lineinfo":
continue
elif components[i] == '-fuse-ld=gold':
elif components[i] == "-fuse-ld=gold":
continue
elif components[i] == '-rdynamic':
elif components[i] == "-fuse-ld=lld":
continue
elif components[i].find("--default-stream") != -1:
continue
elif components[i] == "-rdynamic":
continue
elif components[i] == "-Xfatbin=-compress-all":
continue
elif components[i] == "-forward-unknown-to-host-compiler":
continue
elif (components[i] == '-x' and
components[i+1] == 'cu'):
elif components[i] == "-x" and components[i + 1] == "cu":
# -x cu -> -x cuda
converted_components.append('-x')
converted_components.append('cuda')
components[i+1] = ''
converted_components.append("-x")
converted_components.append("cuda")
components[i + 1] = ""
continue
elif components[i].find('-Xcompiler') != -1:
elif components[i].find("-Xcompiler") != -1:
continue
elif components[i].find('--expt') != -1:
elif components[i].find("--expt-") != -1:
continue
elif components[i].find('-ccbin') != -1:
elif components[i].find("-ccbin") != -1:
continue
elif components[i].find('--generate-code') != -1:
keyword = 'code=sm'
elif components[i].find("--generate-code") != -1:
keyword = "code=sm"
pos = components[i].find(keyword)
capability = components[i][pos + len(keyword) + 1:
pos + len(keyword) + 3]
capability = components[i][
pos + len(keyword) + 1 : pos + len(keyword) + 3
]
if pos != -1:
converted_components.append(
'--cuda-gpu-arch=sm_' + capability)
elif components[i].find('--std=c++14') != -1:
converted_components.append('-std=c++14')
elif components[i].startswith('-isystem='):
converted_components.extend(components[i].split('='))
converted_components.append("--cuda-gpu-arch=sm_" + capability)
elif components[i].find("--std=c++14") != -1:
converted_components.append("-std=c++14")
elif components[i].startswith("-isystem="):
converted_components.extend(components[i].split("="))
else:
converted_components.append(components[i])
converted_components.append('-isystem /usr/local/cuda/include/')
converted_components.append("-isystem /usr/local/cuda/include/")
command = ''
command = ""
for c in converted_components:
command = command + ' ' + c
command = command + " " + c
command = command.strip()
return command
def _configure_flags(self, path, command):
src = os.path.join(self.root_path, 'src')
src = src.replace('/', '\\/')
include = os.path.join(self.root_path, 'include')
include = include.replace('/', '\\/')
def _configure_flags(self, path: str, command: str) -> list[list[str]]:
src = os.path.join(self.root_path, "src").replace("/", "\\/")
include = os.path.join(self.root_path, "include").replace("/", "\\/")
header_filter = '(' + src + '|' + include + ')'
common_args = [self.exe,
"-header-filter=" + header_filter,
'-config='+self.clang_tidy]
common_args.append(path)
common_args.append('--')
header_filter = "(" + src + "|" + include + ")"
common_args = [
self.exe,
path,
"--header-filter=" + header_filter,
"--config-file=" + self.tidy_file,
]
common_args.append("--")
command = self.convert_nvcc_command_to_clang(command)
command = command.split()[1:] # remove clang/c++/g++
if '-c' in command:
index = command.index('-c')
del command[index+1]
command.remove('-c')
if '-o' in command:
index = command.index('-o')
del command[index+1]
command.remove('-o')
command_split = command.split()[1:] # remove clang/c++/g++
if "-c" in command_split:
index = command_split.index("-c")
del command_split[index + 1]
command_split.remove("-c")
if "-o" in command_split:
index = command_split.index("-o")
del command_split[index + 1]
command_split.remove("-o")
common_args.extend(command)
common_args.extend(command_split)
# Two passes, one for device code another for host code.
if path.endswith('cu'):
if path.endswith("cu"):
args = [common_args.copy(), common_args.copy()]
args[0].append('--cuda-host-only')
args[1].append('--cuda-device-only')
args[0].append("--cuda-host-only")
args[1].append("--cuda-device-only")
else:
args = [common_args.copy()]
for a in args:
a.append('-Wno-unused-command-line-argument')
a.append("-Wno-unused-command-line-argument")
return args
def _configure(self):
'''Load and configure compile_commands and clang_tidy.'''
def _configure(self) -> list[list[str]]:
"""Load and configure compile_commands and clang_tidy."""
def should_lint(path):
if not self.cpp_lint and path.endswith('.cc'):
def should_lint(path: str) -> bool:
if not self.cpp_lint and path.endswith(".cc"):
return False
isxgb = path.find('dmlc-core') == -1
isxgb = path.find("dmlc-core") == -1
isxgb = isxgb and (not path.startswith(self.cdb_path))
if isxgb:
print(path)
return True
return False
cdb_file = os.path.join(self.cdb_path, 'compile_commands.json')
with open(cdb_file, 'r') as fd:
cdb_file = os.path.join(self.cdb_path, "compile_commands.json")
with open(cdb_file, "r") as fd:
self.compile_commands = json.load(fd)
tidy_file = os.path.join(self.root_path, '.clang-tidy')
with open(tidy_file) as fd:
self.clang_tidy = yaml.safe_load(fd)
self.clang_tidy = str(self.clang_tidy)
self.tidy_file = os.path.join(self.root_path, ".clang-tidy")
all_files = []
for entry in self.compile_commands:
path = entry['file']
path = entry["file"]
if should_lint(path):
args = self._configure_flags(path, entry['command'])
args = self._configure_flags(path, entry["command"])
all_files.extend(args)
return all_files
def run(self):
'''Run clang-tidy.'''
def run(self) -> bool:
"""Run clang-tidy."""
all_files = self._configure()
passed = True
BAR = '-'*32
BAR = "-" * 32
with Pool(cpu_count()) as pool:
results = pool.map(call, all_files)
for i, (process_status, tidy_status, msg, args) in enumerate(results):
@@ -226,54 +233,50 @@ class ClangTidy(object):
# for cub in thrust is not correct.
if tidy_status == 1:
passed = False
print(BAR, '\n'
'Command args:', ' '.join(args), ', ',
'Process return code:', process_status, ', ',
'Tidy result code:', tidy_status, ', ',
'Message:\n', msg,
BAR, '\n')
print(
BAR,
"\n" "Command args:",
" ".join(args),
", ",
"Process return code:",
process_status,
", ",
"Tidy result code:",
tidy_status,
", ",
"Message:\n",
msg,
BAR,
"\n",
)
if not passed:
print('Errors in `thrust` namespace can be safely ignored.',
'Please address rest of the clang-tidy warnings.')
print(
"Errors in `thrust` namespace can be safely ignored.",
"Please address rest of the clang-tidy warnings.",
)
return passed
def test_tidy(args):
'''See if clang-tidy and our regex is working correctly. There are
many subtleties we need to be careful. For instances:
def test_tidy(args: argparse.Namespace) -> None:
"""See if clang-tidy and our regex is working correctly. There are many subtleties
we need to be careful. Tests here are not thorough, at least we want to guarantee
tidy is not missing anything on the CI.
* Is the string re-directed to pipe encoded as UTF-8? or is it
bytes?
* On Jenkins there's no 'xgboost' directory, are we catching the
right keywords?
* Should we use re.DOTALL?
* Should we use re.MULTILINE?
Tests here are not thorough, at least we want to guarantee tidy is
not missing anything on Jenkins.
'''
"""
root_path = os.path.abspath(os.path.curdir)
tidy_file = os.path.join(root_path, '.clang-tidy')
test_file_path = os.path.join(root_path,
'tests', 'ci_build', 'test_tidy.cc')
tidy_file = os.path.join(root_path, ".clang-tidy")
test_file_path = os.path.join(root_path, "tests", "ci_build", "test_tidy.cc")
with open(tidy_file) as fd:
tidy_config = fd.read()
tidy_config = str(tidy_config)
tidy_config = '-config='+tidy_config
tidy_config = "--config-file=" + tidy_file
if not args.tidy_version:
tidy = 'clang-tidy'
tidy = "clang-tidy"
else:
tidy = 'clang-tidy-' + str(args.tidy_version)
args = [tidy, tidy_config, test_file_path]
(proc_code, tidy_status, error_msg, _) = call(args)
tidy = "clang-tidy-" + str(args.tidy_version)
cmd = [tidy, tidy_config, test_file_path]
(proc_code, tidy_status, error_msg, _) = call(cmd)
assert proc_code == 0
assert tidy_status == 1
print('clang-tidy is working.')
print("clang-tidy is working.")
if __name__ == "__main__":

9
tests/cpp/common/test_intrusive_ptr.cc
View File

@@ -1,3 +1,6 @@
/**
* Copyright 2020-2024, XGBoost contributors
*/
#include <gtest/gtest.h>
#include <xgboost/intrusive_ptr.h>
@@ -12,10 +15,8 @@ class NotCopyConstructible {
NotCopyConstructible &operator=(NotCopyConstructible const &that) = delete;
NotCopyConstructible(NotCopyConstructible&& that) = default;
};
static_assert(
!std::is_trivially_copy_constructible<NotCopyConstructible>::value);
static_assert(
!std::is_trivially_copy_assignable<NotCopyConstructible>::value);
static_assert(!std::is_trivially_copy_constructible_v<NotCopyConstructible>);
static_assert(!std::is_trivially_copy_assignable_v<NotCopyConstructible>);
class ForIntrusivePtrTest {
public:

14
tests/cpp/common/test_span.cc
View File

@@ -1,5 +1,5 @@
/**
* Copyright 2018-2023, XGBoost contributors
* Copyright 2018-2024, XGBoost contributors
*/
#include "test_span.h"
@@ -174,19 +174,11 @@ TEST(Span, FromFirstLast) {
}
}
struct BaseClass {
virtual void operator()() {}
};
struct DerivedClass : public BaseClass {
void operator()() override {}
};
TEST(Span, FromOther) {
// convert constructor
{
Span<DerivedClass> derived;
Span<BaseClass> base { derived };
Span<int> derived;
Span<int const> base{derived};
ASSERT_EQ(base.size(), derived.size());
ASSERT_EQ(base.data(), derived.data());
}

10
tests/cpp/data/test_adapter.cc
View File

@@ -93,11 +93,11 @@ TEST(Adapter, CSCAdapterColsMoreThanRows) {
// A mock for JVM data iterator.
class CSRIterForTest {
std::vector<float> data_ {1, 2, 3, 4, 5};
std::vector<std::remove_pointer<decltype(std::declval<XGBoostBatchCSR>().index)>::type>
feature_idx_ {0, 1, 0, 1, 1};
std::vector<std::remove_pointer<decltype(std::declval<XGBoostBatchCSR>().offset)>::type>
row_ptr_ {0, 2, 4, 5, 5};
std::vector<float> data_{1, 2, 3, 4, 5};
std::vector<std::remove_pointer_t<decltype(std::declval<XGBoostBatchCSR>().index)>> feature_idx_{
0, 1, 0, 1, 1};
std::vector<std::remove_pointer_t<decltype(std::declval<XGBoostBatchCSR>().offset)>> row_ptr_{
0, 2, 4, 5, 5};
size_t iter_ {0};
public:

6
tests/cpp/data/test_sparse_page_dmatrix.cc
View File

@@ -49,7 +49,7 @@ void TestSparseDMatrixLoadFile(Context const* ctx) {
1};
Page out;
for (auto const &page : m.GetBatches<Page>(ctx)) {
if (std::is_same<Page, SparsePage>::value) {
if (std::is_same_v<Page, SparsePage>) {
out.Push(page);
} else {
out.PushCSC(page);
@@ -89,7 +89,7 @@ void TestRetainPage() {
for (auto it = begin; it != end; ++it) {
iterators.push_back(it.Page());
pages.emplace_back(Page{});
if (std::is_same<Page, SparsePage>::value) {
if (std::is_same_v<Page, SparsePage>) {
pages.back().Push(*it);
} else {
pages.back().PushCSC(*it);
@@ -105,7 +105,7 @@ void TestRetainPage() {
// make sure it's const and the caller can not modify the content of page.
for (auto &page : p_fmat->GetBatches<Page>({&ctx})) {
static_assert(std::is_const<std::remove_reference_t<decltype(page)>>::value);
static_assert(std::is_const_v<std::remove_reference_t<decltype(page)>>);
}
}

2
tests/cpp/data/test_sparse_page_dmatrix.cu
View File

@@ -166,7 +166,7 @@ TEST(SparsePageDMatrix, RetainEllpackPage) {
// make sure it's const and the caller can not modify the content of page.
for (auto& page : m->GetBatches<EllpackPage>(&ctx, param)) {
static_assert(std::is_const<std::remove_reference_t<decltype(page)>>::value);
static_assert(std::is_const_v<std::remove_reference_t<decltype(page)>>);
}
// The above iteration clears out all references inside DMatrix.

8
tests/cpp/tree/test_quantile_hist.cc
View File

@@ -62,7 +62,7 @@ void TestPartitioner(bst_target_t n_targets) {
auto ptr = gmat.cut.Ptrs()[split_ind + 1];
float split_value = gmat.cut.Values().at(ptr / 2);
RegTree tree{n_targets, n_features};
if constexpr (std::is_same<ExpandEntry, CPUExpandEntry>::value) {
if constexpr (std::is_same_v<ExpandEntry, CPUExpandEntry>) {
GetSplit(&tree, split_value, &candidates);
} else {
GetMultiSplitForTest(&tree, split_value, &candidates);
@@ -119,7 +119,7 @@ void VerifyColumnSplitPartitioner(bst_target_t n_targets, size_t n_samples,
{
RegTree tree{n_targets, n_features};
CommonRowPartitioner partitioner{&ctx, n_samples, base_rowid, true};
if constexpr (std::is_same<ExpandEntry, CPUExpandEntry>::value) {
if constexpr (std::is_same_v<ExpandEntry, CPUExpandEntry>) {
GetSplit(&tree, min_value, &candidates);
} else {
GetMultiSplitForTest(&tree, min_value, &candidates);
@@ -132,7 +132,7 @@ void VerifyColumnSplitPartitioner(bst_target_t n_targets, size_t n_samples,
{
RegTree tree{n_targets, n_features};
CommonRowPartitioner partitioner{&ctx, n_samples, base_rowid, true};
if constexpr (std::is_same<ExpandEntry, CPUExpandEntry>::value) {
if constexpr (std::is_same_v<ExpandEntry, CPUExpandEntry>) {
GetSplit(&tree, mid_value, &candidates);
} else {
GetMultiSplitForTest(&tree, mid_value, &candidates);
@@ -187,7 +187,7 @@ void TestColumnSplitPartitioner(bst_target_t n_targets) {
auto ptr = gmat.cut.Ptrs()[split_ind + 1];
mid_value = gmat.cut.Values().at(ptr / 2);
RegTree tree{n_targets, n_features};
if constexpr (std::is_same<ExpandEntry, CPUExpandEntry>::value) {
if constexpr (std::is_same_v<ExpandEntry, CPUExpandEntry>) {
GetSplit(&tree, mid_value, &candidates);
} else {
GetMultiSplitForTest(&tree, mid_value, &candidates);