[songgaoye]

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

In C++14 builds, butil::optional<T> uses a custom implementation whose defaulted copy and move operations copy raw storage. For non-trivial types such as std::string, this can cause a double-free.

What is changed and the side effects?

Changed:

• Construct, assign, and destroy the contained value correctly during copy and move operations.
• Make move operations conditionally noexcept based on T.
• Add lifetime regression tests and an explicit C++14 ASan/UBSan CI check.

Side effects:

• Performance effects: Copy and move operations now invoke the contained type's operations instead of copying raw storage.

• Breaking backward compatibility: No intended API break. Copy/move type traits for unsupported operations remain outside the scope of this change.

---

Check List:

• Please make sure your changes are compilable.
• When providing us with a new feature, it is best to add related tests.
• Please follow Contributor Covenant Code of Conduct.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open issues

• These user-defined assignment operators are always declared, which can make optional<T> appear… · New
• This step hard-pins clang++-12 and links -lgtest_main -lgtest without an explicit… · New
• Move construction is now conditionally noexcept, but copy construction remains unconditionally… · New

What changed in this PR

Fixes incorrect lifetime handling in the C++14 butil::optional<T> implementation that could double-free non-trivial types, and adds regression coverage plus CI sanitizer coverage to catch similar issues.

Changes:

• Implement explicit copy/move construction and assignment for engaged optionals (no raw storage copying).
• Add lifetime/assignment-state regression tests including move-only values.
• Add a dedicated C++14 ASan/UBSan CI compilation/run for optional_unittest.

File	Description
test/​optional_unittest.cpp	Adds non-trivial lifetime, assignment-state, and move-only regression tests; enforces C++14 impl when targeted.
src/​butil/​containers/​optional.h	Replaces defaulted copy/move ops with correct value construction/assignment; adds conditional noexcept for moves.
.github/​workflows/​ci-linux.yml	Adds a dedicated C++14 sanitizer run of the optional unit test.

_{Review effort: Lite}

[Copilot]

🟡 Changes recommended

Active moderate and critical findings remain: the converting move path incorrectly resets the source, and the added test contains an assertion that cannot compile.

3 open issues

• Assertion expects move construction from a non-movable type · New
• Converting move incorrectly disengages the source optional · New
• This step hard-pins clang++-12 and links -lgtest_main -lgtest without an explicit…

2 resolved since last review

• These user-defined assignment operators are always declared, which can make optional<T> appear…
• Move construction is now conditionally noexcept, but copy construction remains unconditionally…

_{Review effort: Lite}

[Copilot]

🔵 Needs a closer look

The change leaves optional::swap and free swap unconditionally noexcept even though the new move paths can throw, causing std::terminate for throwing move operations instead of propagating exceptions.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Make swap noexcept conditional on contained type operations

src/​butil/​containers/​optional.h:379

The new move operations now invoke T's move constructor/assignment, but optional::swap (and the free swap) are still unconditionally noexcept. In the empty/non-empty branches, swap routes through this move assignment/value assignment, so a T whose move operation throws now causes std::terminate instead of propagating the exception; the engaged/engaged branch can throw via std::swap as well. Make the swap declarations' exception specification reflect the contained type's swappability/move construction, and update the implementation accordingly.

3 resolved since last review

• Assertion expects move construction from a non-movable type
• Converting move incorrectly disengages the source optional
• This step hard-pins clang++-12 and links -lgtest_main -lgtest without an explicit…

_{Review effort: Lite}

[chenBright]

LGTM