Fix #937: enable returning BehaviorTreeFactory by value by facontidavide · Pull Request #1082 · BehaviorTree/BehaviorTree.CPP · GitHub
Skip to content

Fix #937: enable returning BehaviorTreeFactory by value#1082

Merged
facontidavide merged 2 commits into
masterfrom
fix/937-factory-return-by-value
Feb 1, 2026
Merged

Fix #937: enable returning BehaviorTreeFactory by value#1082
facontidavide merged 2 commits into
masterfrom
fix/937-factory-return-by-value

Conversation

@facontidavide

@facontidavide facontidavide commented Feb 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Moves BehaviorTreeFactory move constructor and move assignment operator definitions from the header (= default) to the .cpp file where the PImpl type is complete
  • This allows the compiler to generate proper move operations for std::unique_ptr<Pimpl>, enabling return-by-value from functions

Fixes #937

Test plan

  • Added BehaviorTreeFactory.ReturnByValue test that verifies factory can be returned from a function
  • All 309 tests pass (308 existing + 1 new)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced internal safety checks for improved robustness.
  • Tests

    • Added expanded test coverage for factory operations.

✏️ Tip: You can customize this high-level summary in your review settings.

Move constructor and move assignment were defaulted in the header where
PImpl is an incomplete type, preventing compilation when returning
BehaviorTreeFactory by value from inline functions. Move the definitions
to bt_factory.cpp where PImpl is complete, matching the existing pattern
used for the destructor.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Feb 1, 2026

Copy link
Copy Markdown

@codecov

codecov Bot commented Feb 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.92%. Comparing base (e0684b0) to head (78ab2fc).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/bt_factory.cpp 50.00% 1 Missing ⚠️
tests/gtest_factory.cpp 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1082      +/-   ##
==========================================
+ Coverage   65.90%   65.92%   +0.01%     
==========================================
  Files         225      225              
  Lines       12727    12739      +12     
  Branches     1186     1186              
==========================================
+ Hits         8388     8398      +10     
- Misses       4289     4291       +2     
  Partials       50       50              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Use explicit != nullptr comparison for pointer-to-bool conversion
flagged by clang-tidy.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@facontidavide facontidavide marked this pull request as ready for review February 1, 2026 19:37
@facontidavide facontidavide merged commit 1217c7e into master Feb 1, 2026
14 of 15 checks passed
@facontidavide facontidavide deleted the fix/937-factory-return-by-value branch February 1, 2026 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Is returning a local instance of BehaviorTreeFactory by value from a function disallowed ?

1 participant