Skip to content

Commit a3f853a

Browse files
facontidavideclaude
andcommitted
Fix CI failures: TSAN race, clang-tidy, and nodiscard warnings
- Fix TSAN race condition in FileLogger2: set ready flag AFTER acquiring mutex for the first time to establish proper synchronization - Add deleted copy operations to FileLogger2PImplBase to satisfy cppcoreguidelines-special-member-functions - Fix nodiscard warnings in tests by casting convertFromString return values to void in ASSERT_THROW statements Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent b41b8b6 commit a3f853a

File tree

4 files changed

+16
-8
lines changed

4 files changed

+16
-8
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,3 +26,4 @@ t11_groot_howto.btlog
2626
minitrace.json
2727
/.worktrees/*
2828
/docs/plans/*
29+
/coverage_report/*

include/behaviortree_cpp/loggers/bt_file_logger_v2.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ struct FileLogger2PImplBase
2222
std::unique_ptr<FileLogger2Private> _p;
2323
FileLogger2PImplBase();
2424
~FileLogger2PImplBase();
25+
FileLogger2PImplBase(const FileLogger2PImplBase&) = delete;
26+
FileLogger2PImplBase& operator=(const FileLogger2PImplBase&) = delete;
2527
FileLogger2PImplBase(FileLogger2PImplBase&&) = default;
2628
FileLogger2PImplBase& operator=(FileLogger2PImplBase&&) = default;
2729
};

src/loggers/bt_file_logger_v2.cpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,17 @@ void FileLogger2::flush()
132132

133133
void FileLogger2::writerLoop()
134134
{
135-
// Signal that the writer thread is ready to accept callbacks
136-
_p->ready.store(true, std::memory_order_release);
137-
138135
// local buffer in this thread
139136
std::deque<Transition> transitions;
140137

138+
// Signal that the writer thread is ready to accept callbacks.
139+
// IMPORTANT: This must happen AFTER acquiring the mutex for the first time,
140+
// to establish proper synchronization with the callback thread.
141+
{
142+
std::scoped_lock lock(_p->queue_mutex);
143+
_p->ready.store(true, std::memory_order_release);
144+
}
145+
141146
while(_p->loop)
142147
{
143148
transitions.clear();

tests/gtest_basic_types.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,8 @@ TEST(BasicTypes, ConvertFromString_Int)
9797
ASSERT_EQ(convertFromString<int>("-42"), -42);
9898
ASSERT_EQ(convertFromString<int>("0"), 0);
9999

100-
ASSERT_THROW(convertFromString<int>("not_a_number"), RuntimeError);
101-
ASSERT_THROW(convertFromString<int>(""), RuntimeError);
100+
ASSERT_THROW((void)convertFromString<int>("not_a_number"), RuntimeError);
101+
ASSERT_THROW((void)convertFromString<int>(""), RuntimeError);
102102
}
103103

104104
TEST(BasicTypes, ConvertFromString_Int64)
@@ -120,7 +120,7 @@ TEST(BasicTypes, ConvertFromString_Double)
120120
ASSERT_DOUBLE_EQ(convertFromString<double>("0.0"), 0.0);
121121

122122
// Invalid double throws std::invalid_argument
123-
ASSERT_THROW(convertFromString<double>("not_a_number"), std::invalid_argument);
123+
ASSERT_THROW((void)convertFromString<double>("not_a_number"), std::invalid_argument);
124124
}
125125

126126
TEST(BasicTypes, ConvertFromString_Bool)
@@ -135,7 +135,7 @@ TEST(BasicTypes, ConvertFromString_Bool)
135135
ASSERT_FALSE(convertFromString<bool>("FALSE"));
136136
ASSERT_FALSE(convertFromString<bool>("0"));
137137

138-
ASSERT_THROW(convertFromString<bool>("invalid"), RuntimeError);
138+
ASSERT_THROW((void)convertFromString<bool>("invalid"), RuntimeError);
139139
}
140140

141141
TEST(BasicTypes, ConvertFromString_String)
@@ -153,7 +153,7 @@ TEST(BasicTypes, ConvertFromString_NodeStatus)
153153
ASSERT_EQ(convertFromString<NodeStatus>("IDLE"), NodeStatus::IDLE);
154154
ASSERT_EQ(convertFromString<NodeStatus>("SKIPPED"), NodeStatus::SKIPPED);
155155

156-
ASSERT_THROW(convertFromString<NodeStatus>("INVALID"), RuntimeError);
156+
ASSERT_THROW((void)convertFromString<NodeStatus>("INVALID"), RuntimeError);
157157
}
158158

159159
TEST(BasicTypes, ConvertFromString_NodeType)

0 commit comments

Comments
 (0)