carbon-language/carbon-lang
1
0
Fork 0
mirror of https://github.com/carbon-language/carbon-lang.git synced 2026-09-24 19:20:13 +01:00
Code Issues Packages Projects Releases 10 Wiki Activity

Fix outstanding clang-tidy errors. (#3654)

Browse Source 
Recent runs of `clang-tidy` for me started showing more errors, and this
is a collection of changes to address them.

First, I've systematically applied the disabling tag to all C++ rules
under //explorer/... with `buildozer` so we don't spend time analyzing
this code or reporting errors from it. Not sure this was strictly
necessary, but it seemed like a nice consistency improvement.

Next, I disabled a buggy check for missing `default` cases in
`switch`es. It seems to get confused by the fancy conversions in our
`enum_base.h`. We don't miss much with this as the Clang compiler
warnings for `switch` catch most of our actual bugs. I also removed the
local disabling of this now that it is turned off centrally.

Lastly, I added error checking to two file descriptor manipulating calls
in the `file_test` infrastructure.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This commit is contained in:
Chandler Carruth
2024-01-26 18:20:47 +00:00
committed by GitHub
co-authored by Jon Ross-Perkins
parent c1a894fca7
commit 13de9e9d06
17 changed files with 152 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.clang-tidy
+4 -2
View File
@@ -16,6 +16,8 @@ WarningsAsErrors: '*'
# inst_id = name_ref->value_id;
# ^ unchecked access to optional value
# }
# - bugprone-switch-missing-default-case has false positives for `enum_base.h`.
# Clang's built-in switch warnings cover most of our risk of bugs here.
# - google-readability-function-size overlaps with readability-function-size.
# - modernize-use-nodiscard is disabled because it only fixes const methods,
# not non-const, which yields distracting results on accessors.
@@ -24,8 +26,8 @@ WarningsAsErrors: '*'
Checks:
-*, bugprone-*, -bugprone-branch-clone, -bugprone-easily-swappable-parameters,
-bugprone-exception-escape, -bugprone-narrowing-conversions,
-bugprone-unchecked-optional-access, google-*,
-google-readability-function-size, -google-readability-todo,
-bugprone-switch-missing-default-case, -bugprone-unchecked-optional-access,
google-*, -google-readability-function-size, -google-readability-todo,
misc-definitions-in-headers, misc-misplaced-const, misc-redundant-expression,
misc-static-assert, misc-unconventional-assign-operator,
misc-uniqueptr-reset-release, misc-unused-*, modernize-*,
explorer/BUILD
+6
View File
@@ -22,6 +22,9 @@ cc_library(
name = "main",
srcs = ["main.cpp"],
hdrs = ["main.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//common:error",
"//common:ostream",
@@ -35,6 +38,9 @@ cc_binary(
name = "explorer",
srcs = ["main_bin.cpp"],
env = cc_env(),
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":main",
"//common:bazel_working_dir",
explorer/ast/BUILD
+21
View File
@@ -74,6 +74,9 @@ cc_library(
"ast_test_matchers_internal.h",
],
hdrs = ["ast_test_matchers.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":ast",
"@com_google_googletest//:gtest",
@@ -85,6 +88,9 @@ cc_test(
name = "ast_test_matchers_test",
size = "small",
srcs = ["ast_test_matchers_test.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":ast",
":ast_test_matchers",
@@ -114,11 +120,17 @@ cc_test(
cc_library(
name = "library_name",
hdrs = ["library_name.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
)
cc_library(
name = "paren_contents",
hdrs = ["paren_contents.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//explorer/base:error_builders",
"//explorer/base:source_location",
@@ -129,6 +141,9 @@ cc_test(
name = "element_test",
size = "small",
srcs = ["element_test.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":ast",
":paren_contents",
@@ -160,6 +175,9 @@ cc_library(
name = "static_scope",
srcs = ["static_scope.cpp"],
hdrs = ["static_scope.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":ast",
"//common:check",
@@ -177,5 +195,8 @@ cc_library(
cc_library(
name = "expression_category",
hdrs = ["expression_category.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = ["@llvm-project//llvm:Support"],
)
explorer/base/BUILD
+30
View File
@@ -35,6 +35,9 @@ cc_test(
cc_library(
name = "decompose",
hdrs = ["decompose.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"@llvm-project//llvm:Support",
],
@@ -44,6 +47,9 @@ cc_test(
name = "decompose_test",
size = "small",
srcs = ["decompose_test.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":decompose",
"//testing/base:gtest_main",
@@ -54,6 +60,9 @@ cc_test(
cc_library(
name = "error_builders",
hdrs = ["error_builders.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":source_location",
"//common:error",
@@ -64,6 +73,9 @@ cc_test(
name = "error_builders_test",
size = "small",
srcs = ["error_builders_test.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":error_builders",
":source_location",
@@ -76,6 +88,9 @@ cc_test(
cc_library(
name = "nonnull",
hdrs = ["nonnull.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//common:check",
],
@@ -84,6 +99,9 @@ cc_library(
cc_library(
name = "print_as_id",
hdrs = ["print_as_id.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//common:ostream",
],
@@ -92,6 +110,9 @@ cc_library(
cc_library(
name = "source_location",
hdrs = ["source_location.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":nonnull",
"//common:ostream",
@@ -101,6 +122,9 @@ cc_library(
cc_library(
name = "trace_stream",
hdrs = ["trace_stream.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":source_location",
"//common:check",
@@ -114,6 +138,9 @@ cc_test(
name = "set_program_phase_raii_test",
size = "small",
srcs = ["set_program_phase_raii_test.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":trace_stream",
"//testing/base:gtest_main",
@@ -125,6 +152,9 @@ cc_test(
name = "set_file_context_raii_test",
size = "small",
srcs = ["set_file_context_raii_test.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":source_location",
":trace_stream",
explorer/fuzzing/BUILD
+16 -3
View File
@@ -10,6 +10,9 @@ cc_library(
testonly = 1,
srcs = ["ast_to_proto.cpp"],
hdrs = ["ast_to_proto.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//explorer/ast",
"//testing/fuzzing:carbon_cc_proto",
@@ -21,6 +24,9 @@ cc_binary(
name = "ast_to_proto",
testonly = 1,
srcs = ["ast_to_proto_main.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":ast_to_proto_lib",
"//common:bazel_working_dir",
@@ -45,6 +51,9 @@ cc_test(
"//explorer:carbon_files",
"//explorer:standard_libraries",
],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":ast_to_proto_lib",
"//explorer/syntax",
@@ -61,6 +70,9 @@ cc_library(
testonly = 1,
srcs = ["fuzzer_util.cpp"],
hdrs = ["fuzzer_util.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//common:check",
"//common:error",
@@ -81,6 +93,9 @@ cc_test(
data = [
"//explorer:standard_libraries",
],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":fuzzer_util",
"//testing/base:gtest_main",
@@ -98,10 +113,8 @@ cc_fuzz_test(
corpus = glob(["fuzzer_corpus/*"]),
shard_count = 8,
tags = [
"proto-fuzzer",
# Running clang-tidy is slow, and explorer is currently feature frozen,
# so don't spend time linting it.
"no-clang-tidy",
"proto-fuzzer",
],
deps = [
":fuzzer_util",
explorer/interpreter/BUILD
+39
View File
@@ -42,6 +42,9 @@ cc_library(
hdrs = [
"action_stack.h",
],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":action",
":stack",
@@ -57,6 +60,9 @@ cc_library(
cc_library(
name = "dictionary",
hdrs = ["dictionary.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = ["//explorer/base:arena"],
)
@@ -64,6 +70,9 @@ cc_library(
name = "exec_program",
srcs = ["exec_program.cpp"],
hdrs = ["exec_program.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":interpreter",
":resolve_control_flow",
@@ -84,6 +93,9 @@ cc_library(
name = "heap",
srcs = ["heap.cpp"],
hdrs = ["heap.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":action",
":heap_allocation_interface",
@@ -102,6 +114,9 @@ cc_library(
cc_library(
name = "heap_allocation_interface",
hdrs = ["heap_allocation_interface.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//common:error",
"//explorer/ast",
@@ -147,6 +162,9 @@ cc_library(
name = "resolve_control_flow",
srcs = ["resolve_control_flow.cpp"],
hdrs = ["resolve_control_flow.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//common:check",
"//explorer/ast",
@@ -162,6 +180,9 @@ cc_library(
name = "resolve_names",
srcs = ["resolve_names.cpp"],
hdrs = ["resolve_names.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":stack_space",
"//common:check",
@@ -177,6 +198,9 @@ cc_library(
cc_library(
name = "stack",
hdrs = ["stack.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = ["//common:check"],
)
@@ -188,6 +212,9 @@ cc_library(
hdrs = [
"pattern_analysis.h",
],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":action",
"//common:error",
@@ -249,6 +276,9 @@ cc_library(
hdrs = [
"resolve_unformed.h",
],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":stack_space",
"//common:check",
@@ -266,6 +296,9 @@ cc_library(
name = "stack_space",
srcs = ["stack_space.cpp"],
hdrs = ["stack_space.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//common:check",
"//common:error",
@@ -301,6 +334,9 @@ cc_library(
hdrs = [
"type_utils.h",
],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//explorer/ast",
"//explorer/base:nonnull",
@@ -316,6 +352,9 @@ cc_library(
hdrs = [
"pattern_match.h",
],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":action",
":type_utils",
explorer/parse_and_execute/BUILD
+6
View File
@@ -13,6 +13,9 @@ cc_library(
name = "parse_and_execute",
srcs = ["parse_and_execute.cpp"],
hdrs = ["parse_and_execute.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
"//common:check",
"//common:error",
@@ -29,6 +32,9 @@ cc_test(
name = "parse_and_execute_test",
size = "small",
srcs = ["parse_and_execute_test.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":parse_and_execute",
"//testing/base:gtest_main",
explorer/syntax/BUILD
+18
View File
@@ -10,6 +10,9 @@ package(default_visibility = ["//explorer/parse_and_execute:__pkg__"])
cc_library(
name = "bison_wrap",
hdrs = ["bison_wrap.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = ["//common:check"],
)
@@ -17,6 +20,9 @@ cc_test(
name = "parse_test",
size = "small",
srcs = ["parse_test.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":syntax",
"//explorer/base:arena",
@@ -30,6 +36,9 @@ cc_library(
testonly = 1,
srcs = ["parse_test_matchers_internal.h"],
hdrs = ["parse_test_matchers.h"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":syntax",
"@com_google_googletest//:gtest",
@@ -42,6 +51,9 @@ cc_library(
srcs = ["prelude.cpp"],
hdrs = ["prelude.h"],
data = ["//explorer:standard_libraries"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":syntax",
"//common:error",
@@ -77,6 +89,9 @@ cc_library(
"-Wno-unused-function",
"-Wno-writable-strings",
],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
visibility = [
"//explorer/fuzzing:__pkg__",
"//explorer/parse_and_execute:__pkg__",
@@ -150,6 +165,9 @@ cc_test(
name = "unimplemented_example_test",
size = "small",
srcs = ["unimplemented_example_test.cpp"],
# Running clang-tidy is slow, and explorer is currently feature frozen, so
# don't spend time linting it.
tags = ["no-clang-tidy"],
deps = [
":parse_test_matchers",
":syntax",
testing/file_test/file_test_base.cpp
+12 -2
View File
@@ -729,8 +729,18 @@ static auto Main(int argc, char** argv) -> int {
// Tests might try to read from stdin. Ensure those reads fail by closing
// stdin and reopening it as /dev/null. Note that STDIN_FILENO doesn't exist
// on Windows, but POSIX requires it to be 0.
llvm::sys::Process::SafelyCloseFileDescriptor(0);
llvm::sys::Process::FixupStandardFileDescriptors();
if (std::error_code error =
llvm::sys::Process::SafelyCloseFileDescriptor(0)) {
llvm::errs() << "Unable to close standard input: " << error.message()
<< "\n";
return EXIT_FAILURE;
}
if (std::error_code error =
llvm::sys::Process::FixupStandardFileDescriptors()) {
llvm::errs() << "Unable to correct standard file descriptors: "
<< error.message() << "\n";
return EXIT_FAILURE;
}
llvm::SmallVector<std::string> tests = GetTests();
auto test_factory = GetFileTestFactory();
toolchain/check/check.cpp
-2
View File
@@ -206,8 +206,6 @@ static auto ProcessParseNodes(Context& context,
ErrorTrackingDiagnosticConsumer& err_tracker)
-> bool {
for (auto parse_node : context.parse_tree().postorder()) {
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (auto parse_kind = context.parse_tree().node_kind(parse_node)) {
#define CARBON_PARSE_NODE_KIND(Name) \
case Parse::NodeKind::Name: { \
toolchain/check/context.cpp
-5
View File
@@ -799,8 +799,6 @@ class TypeCompleter {
CARBON_CHECK(xref_inst.kind() == SemIR::Builtin::Kind)
<< "TODO: Handle other kinds of inst cross-references";
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (xref_inst.As<SemIR::Builtin>().builtin_kind) {
case SemIR::BuiltinKind::TypeType:
case SemIR::BuiltinKind::Error:
@@ -919,9 +917,6 @@ class TypeCompleter {
// dedicated file-scope instruction block where possible, or somewhere else
// that better reflects the definition of the type, rather than wherever the
// type happens to first be required to be complete.
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (inst.kind()) {
case SemIR::AddrOf::Kind:
case SemIR::AddrPattern::Kind:
toolchain/check/eval.cpp
-3
View File
@@ -288,9 +288,6 @@ auto TryEvalInst(Context& context, SemIR::InstId inst_id, SemIR::Inst inst)
-> SemIR::ConstantId {
// TODO: Ensure we have test coverage for each of these cases that can result
// in a constant, once those situations are all reachable.
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (inst.kind()) {
// These cases are constants if their operands are.
case SemIR::AddrOf::Kind:
toolchain/lower/function_context.cpp
-2
View File
@@ -41,8 +41,6 @@ auto FunctionContext::LowerBlock(SemIR::InstBlockId block_id) -> void {
for (const auto& inst_id : sem_ir().inst_blocks().Get(block_id)) {
auto inst = sem_ir().insts().Get(inst_id);
CARBON_VLOG() << "Lowering " << inst_id << ": " << inst << "\n";
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (inst.kind()) {
#define CARBON_SEM_IR_INST_KIND(Name) \
case SemIR::Name::Kind: \
toolchain/parse/parse.cpp
-2
View File
@@ -36,8 +36,6 @@ auto Parse(Lex::TokenizedBuffer& tokens, DiagnosticConsumer& consumer,
context.PushState(State::DeclScopeLoop);
while (!context.state_stack().empty()) {
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (context.state_stack().back().state) {
#define CARBON_PARSE_STATE(Name) \
case State::Name: \
toolchain/sem_ir/file.cpp
-6
View File
@@ -199,8 +199,6 @@ auto File::OutputYaml(bool include_builtins) const -> Yaml::OutputMapping {
// precedence of that type's syntax. Higher numbers correspond to higher
// precedence.
static auto GetTypePrecedence(InstKind kind) -> int {
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (kind) {
case ArrayType::Kind:
case BindSymbolicName::Kind:
@@ -311,8 +309,6 @@ auto File::StringifyTypeExpr(InstId outer_inst_id) const -> std::string {
}
auto inst = insts().Get(step.inst_id);
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (inst.kind()) {
case ArrayType::Kind: {
auto array = inst.As<ArrayType>();
@@ -501,8 +497,6 @@ auto GetExprCategory(const File& file, InstId inst_id) -> ExprCategory {
while (true) {
auto inst = ir->insts().Get(inst_id);
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (inst.kind()) {
case Assign::Kind:
case BaseDecl::Kind:
toolchain/sem_ir/formatter.cpp
-2
View File
@@ -753,8 +753,6 @@ class Formatter {
}
auto FormatInstruction(InstId inst_id, Inst inst) -> void {
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (inst.kind()) {
#define CARBON_SEM_IR_INST_KIND(InstT) \
case InstT::Kind: \
toolchain/sem_ir/inst.cpp
-2
View File
@@ -19,8 +19,6 @@ auto Inst::Print(llvm::raw_ostream& out) const -> void {
}
};
// clang warns on unhandled enum values; clang-tidy is incorrect here.
// NOLINTNEXTLINE(bugprone-switch-missing-default-case)
switch (kind_) {
#define CARBON_SEM_IR_INST_KIND(Name) \
case Name::Kind: \